mbed TLS 1.3: Fix compilation warnings with VS2015 x64 #999

andresag01 · 2017-07-06T13:44:25Z

Fix 6 compilation warnings in mbed TLS 1.3 in the sample programs and the net.c module. All the warnings are related to conversions from size_t to int, which can result in a loss of data. The fixes are essentially small backports from the code in the mbed TLS development branch.

The warning was caused because of conversions from size_t to int, which can cause data loss. The files affected are: * ssl_client2.c * ssl_server2.c * ssl_mail_client.c

The warning was caused because in MSVC some of the function parameters for the socket APIs are int while the fields in struct addrinfo are size_t e.g. possible data loss.

hanno-becker · 2017-07-06T15:20:08Z

library/net.c

@@ -92,6 +92,14 @@ static int wsa_init_done = 0;

 #endif /* ( _WIN32 || _WIN32_WCE ) && !EFIX64 && !EFI32 */

+/* Some MS functions want int and MSVC warns if we pass size_t,
+ * but the standard fucntions use socklen_t, so cast only for MSVC */
+#if defined(_MSC_VER)


There's a typo here.

I copied that from the development branch, so the typo comes from there. I will fix the typo here but I do not think its worth creating a PR for development and mbed TLS 2.1.

Yes, I just saw that. Agreed, we can fix the typo in 2.1 and development on other occasions.

andresag01 · 2017-07-06T15:27:35Z

@hanno-arm: Fixed the type in library/net.c

hanno-becker · 2017-07-06T15:31:11Z

programs/ssl/ssl_mail_client.c

-#define read _read
-#define write _write
+#define read(fd, buf, len)  _read( fd, (void *)buf, (unsigned int)len )
+#define write(fd, buf, len) _write( fd, (const void *)buf, (unsigned int)len )
 #endif


Could you add brackets around the arguments to ensure the casts also work as intended if compound expressions are passed?

Again, I copied this from the development branch: https://github.com/ARMmbed/mbedtls/blob/development/library/net_sockets.c#L66

andresag01 · 2017-07-06T15:58:22Z

@hanno-arm: Added the brackets around the buf and len arguments.

RonEld

Casting should be done in specific cases opnly, and not to be used commonly to fix compilation warnings. Personally, I prefer to use casting only when there is no other choice, and do it carefully..

RonEld · 2017-07-11T11:44:06Z

programs/ssl/ssl_mail_client.c

@@ -57,8 +57,10 @@
 #include <unistd.h>
 #else
 #include <io.h>
-#define read _read
-#define write _write
+#define read(fd, buf, len)                                      \


personally, I think the MACRO checks should be different, to make it more readable(although this is not part of this PR):

#if defined (_MSC_VER) #else #endif

ok, I think I will skip this change for this PR.

RonEld · 2017-07-11T11:44:51Z

programs/ssl/ssl_mail_client.c

-#define read _read
-#define write _write
+#define read(fd, buf, len)                                      \
+    _read( fd, (void *)( buf ), (unsigned int)( len ) )


Theoretically, we could lose data here. parameter len is size_t, which is 64 bit in x64. if for some reason, len would be MAX_UINT, we will downcast to unsigned int and lose information. This casting only removes the warning(symptom), not the data loss.

Yes, we could. In fact, I copied this code from the development branch: https://github.com/ARMmbed/mbedtls/blob/development/library/net_sockets.c#L67. I do not think this is a major problem as this is only sample code (even the net_sockets.c), but perhaps we could improve the quality of this code. Do you think I should raise an issue on this or fix it here?

Yes, we should raise an issue for this, but if possible, fis it here while we are modifying this file

The problem in this case is that in Unix we use the function read() while in Windows we use the function _read. Annoyingly the signature for these two functions is different, hence why we use the cast in the Window case as the type of len in linux is size_t. We could try to add some checks to make sure that the input parameter is not overflowed as you say, but we would need to use #if guards and I think that this just complicates the sample code...

at least comment this, so people using this sample code would be aware of this risk

RonEld · 2017-07-11T11:55:09Z

programs/ssl/ssl_server2.c

@@ -1791,7 +1791,7 @@ int main( int argc, char *argv[] )
            unsigned char *larger_buf;

            ori_len = ret;
-            extra_len = ssl_get_bytes_avail( &ssl );
+            extra_len = (int) ssl_get_bytes_avail( &ssl );


I think it is better to change extra_len to size_t type, and thus remove the need for casting

I think that is a good idea, however everything I have done here is backport the fix from the development branch. Do you think I should try to fix the problem here or raise another issue to address this in all the 3 branches?

I think both:)
I think you should fix here, as we are already modifying the file, and raise another issuer for the other two branches

@RonEld: Even if we change this to size_t we will eventually need to cast as the return type of ssl_read() is int. In any case, we will not overflow the buffer because according to the RFC 5246 the length of the TLS record's content cannot exceed 2^14.

hanno-becker · 2017-07-11T13:35:26Z

I'm ok with the conversion inside the test applications. As for the explicit unsigned-to-signed cast in net.c for MSVC, I think we can omit the bounds check there because if the windows API returns an addr_len outside the value range of int, there's already something broken at the OS level (though, admittedly, not broken in the strict sense of a violation of the specification).

In general, I think we should open an issue on systematically investigating all compiler warnings that we receive when compiling with GCC and -Wconversion enabled. This would e.g. also include the issue @RonEld raised about extra_len, because it is later used as part of the third argument to memset, which is expected to be a size_t.

RonEld · 2017-07-11T13:47:53Z

programs/ssl/ssl_client2.c

@@ -1267,7 +1267,7 @@ int main( int argc, char *argv[] )

    len = polarssl_snprintf( (char *) buf, sizeof(buf) - 1, GET_REQUEST,
                    opt.request_page );
-    tail_len = strlen( GET_REQUEST_END );
+    tail_len = (int) strlen( GET_REQUEST_END );


we should consider changing tail_len to size_t

I looked at the code again and I think that if we change this to size_t we will eventually need to use casts back to int. The problem is that functions such as snprintf() return int. Also, note that all the input parameters, etc are of type int so its not that straight forward to change one type. I think its best if we just investigate these problems as part of #1007

hanno-becker · 2017-07-11T13:56:51Z

@andresag01 Would you mind forward-porting the changes in net.c and ssl_mail_client.c? I think it's best to do it now instead of opening another issue or forgetting about it altogether.

RonEld

I approve this, however we should investigate further the need for casting

andresag01 · 2017-07-12T12:56:19Z

@RonEld: I agree, @hanno-arm has opened a separate issue to look into the casting #1007

andresag01 · 2017-07-31T15:46:49Z

@RonEld, @hanno-arm: Please note that I have fixed a further compilation warning in x509.c x509_parse_int() and added a ChangeLog entry.

RonEld · 2017-08-01T06:26:41Z

@andresag01 please investigate CI failures

hanno-becker · 2017-09-11T06:28:48Z

@RonEld I think the CI doesn't work for maintenance branches. Could you check @andresag01 changes from July 31th (added since your last approval) and put the PR to Waiting for Approval state if you're fine?

RonEld · 2017-09-11T07:45:15Z

@hanno-arm I know the CI doesn't work for CI builds. I don't know why I made that comment...
Anyway, I added some comments, but not blocking.

simonbutcher · 2017-10-07T22:25:08Z

Reviewed, tested, approved and merged with mbedtls-1.3.

Threat model summary 2.28

Andres Amaya Garcia added 2 commits July 6, 2017 14:36

Fix MSVC warning in sample programs

83081c5

The warning was caused because of conversions from size_t to int, which can cause data loss. The files affected are: * ssl_client2.c * ssl_server2.c * ssl_mail_client.c

Fix MSVC warning in net.c

2a0344f

The warning was caused because in MSVC some of the function parameters for the socket APIs are int while the fields in struct addrinfo are size_t e.g. possible data loss.

hanno-becker reviewed Jul 6, 2017

View reviewed changes

Fix type in net.c comment

c4b659c

hanno-becker reviewed Jul 6, 2017

View reviewed changes

Add brackets around function macro arguments

e8c587e

andresag01 requested review from hanno-becker and RonEld July 11, 2017 09:20

RonEld suggested changes Jul 11, 2017

View reviewed changes

RonEld added the mbed TLS team label Jul 11, 2017

RonEld reviewed Jul 11, 2017

View reviewed changes

hanno-becker mentioned this pull request Jul 11, 2017

Investigate integer conversion warnings #1007

Closed

hanno-becker approved these changes Jul 11, 2017

View reviewed changes

RonEld approved these changes Jul 12, 2017

View reviewed changes

Add brackets around net.c macro arguments

5ad6be7

This was referenced Jul 12, 2017

Fix typo and bracketing in macro args #1012

Merged

Forwardport 2.1: Fix typo and bracketing in macro args #1013

Closed

Andres Amaya Garcia added 3 commits July 31, 2017 16:37

Change param type for x509_parse_int() to fix warn

28d7d3a

Fix coding style in x509_parse_int()

e4fd1ba

Add ChangeLog entry

eb01d3c

hanno-becker approved these changes Sep 11, 2017

View reviewed changes

hanno-becker added the bug label Sep 11, 2017

RonEld added the needs-design-approval label Sep 11, 2017

simonbutcher closed this Oct 7, 2017

gilles-peskine-arm removed the needs-design-approval label Nov 24, 2017

paul-elliott-arm pushed a commit that referenced this pull request Mar 28, 2023

Merge pull request #999 from yanesca/threat_model_summary-2.28

b3b9059

Threat model summary 2.28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mbed TLS 1.3: Fix compilation warnings with VS2015 x64 #999

mbed TLS 1.3: Fix compilation warnings with VS2015 x64 #999

andresag01 commented Jul 6, 2017

hanno-becker Jul 6, 2017

andresag01 Jul 6, 2017

hanno-becker Jul 6, 2017

andresag01 commented Jul 6, 2017

hanno-becker Jul 6, 2017

andresag01 Jul 6, 2017

andresag01 commented Jul 6, 2017

RonEld left a comment

RonEld Jul 11, 2017

andresag01 Jul 11, 2017

RonEld Jul 11, 2017

andresag01 Jul 11, 2017

RonEld Jul 11, 2017

andresag01 Jul 12, 2017

RonEld Sep 11, 2017

RonEld Jul 11, 2017

andresag01 Jul 11, 2017

RonEld Jul 11, 2017

andresag01 Jul 12, 2017

hanno-becker commented Jul 11, 2017

RonEld Jul 11, 2017

andresag01 Jul 12, 2017

hanno-becker commented Jul 11, 2017

RonEld left a comment

andresag01 commented Jul 12, 2017

andresag01 commented Jul 31, 2017

RonEld commented Aug 1, 2017

hanno-becker commented Sep 11, 2017

RonEld commented Sep 11, 2017

simonbutcher commented Oct 7, 2017

mbed TLS 1.3: Fix compilation warnings with VS2015 x64 #999

mbed TLS 1.3: Fix compilation warnings with VS2015 x64 #999

Conversation

andresag01 commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Jul 6, 2017

RonEld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker commented Jul 11, 2017

RonEld left a comment

Choose a reason for hiding this comment

andresag01 commented Jul 12, 2017

andresag01 commented Jul 31, 2017

RonEld commented Aug 1, 2017

hanno-becker commented Sep 11, 2017

RonEld commented Sep 11, 2017

simonbutcher commented Oct 7, 2017