Verify numerical ip SubjectAlternativeName properly

[ekoby]

Description

Fixes TLS verification failure when trying to validate server connected to by IP address and server presents certificate with IP Subject Alternate Name

Status

READY

Requires Backporting

[edited by mpg]: NO - this is a new feature

Todos

• [x] Tests
• [x] Documentation
• [x] Changelog updated

Steps to test or reproduce

try connecting to CloudFlare 1.1.1.1 server. 1.1.1.1 or 162.159.132.53 IP addresses can be used

[RonEld]

@ekoby Thank you for your contribution!

We have seen you have sent your CLA acceptance, we will process it. However, since this is an enhancement which doesn't require backporting, and will be merged to an Apache only branch, there is no need for a CLA to this specific PR

[RonEld]

CI failed in the following:

******************************************
* Testing configuration: config-suite-b.h
******************************************

/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c: In function ‘x509_crt_check_cn’:
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2969:24: error: storage size of ‘ipv4’ isn’t known
         struct in_addr ipv4;
                        ^
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2970:25: error: storage size of ‘ipv6’ isn’t known
         struct in6_addr ipv6;
                         ^
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2973:13: error: implicit declaration of function ‘inet_pton’ [-Werror=implicit-function-declaration]
             inet_pton( AF_INET, cn, &ipv4 ) == 1 &&
             ^
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2973:24: error: ‘AF_INET’ undeclared (first use in this function)
             inet_pton( AF_INET, cn, &ipv4 ) == 1 &&
                        ^
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2973:24: note: each undeclared identifier is reported only once for each function it appears in
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2978:29: error: ‘AF_INET6’ undeclared (first use in this function)
         else if( inet_pton( AF_INET6, cn, &ipv6 ) == 1 &&
                             ^
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2970:25: error: unused variable ‘ipv6’ [-Werror=unused-variable]
         struct in6_addr ipv6;
                         ^
/home/travis/build/ARMmbed/mbedtls/library/x509_crt.c:2969:24: error: unused variable ‘ipv4’ [-Werror=unused-variable]
         struct in_addr ipv4;
                        ^
cc1: all warnings being treated as errors
library/CMakeFiles/mbedx509.dir/build.make:127: recipe for target 'library/CMakeFiles/mbedx509.dir/x509_crt.c.o' 

Please fix the build error.

[ekoby]

Please fix the build error.

Is there any way to run those builds/tests on my own?

[RonEld]

Is there any way to run those builds/tests on my own?

There are several other failures now. For example, on Ubuntu16.04 with arm-none-eabi-gcc toolchain:

x509_crt.c:90:24: fatal error: netinet/in.h: No such file or directory

With win32-mingw:

In file included from C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\src\library\x509_crt.c:87:0:

C:/tools/mingw64/x86_64-w64-mingw32/include/winsock2.h:15:2: warning: #warning Please include winsock2.h before windows.h [-Wcpp]

 #warning Please include winsock2.h before windows.h

  ^

C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\src\library\x509_crt.c: In function 'x509_crt_check_cn':

C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\src\library\x509_crt.c:2973:25: error: storage size of 'ipv6' isn't known

         struct in6_addr ipv6;

                         ^

C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\src\library\x509_crt.c:88:29: warning: implicit declaration of function 'InetPtonW' [-Wimplicit-function-declaration]

 #define inet_pton(f,a,addr) InetPtonW(f,addr,a,strlen(a))

                             ^

C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\src\library\x509_crt.c:2976:13: note: in expansion of macro 'inet_pton'

             inet_pton( AF_INET, cn, &ipv4 ) == 1 &&

             ^

C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\src\library\x509_crt.c:2973:25: warning: unused variable 'ipv6' [-Wunused-variable]

         struct in6_addr ipv6;

                         ^

library\CMakeFiles\mbedx509.dir\build.make:187: recipe for target 'library/CMakeFiles/mbedx509.dir/x509_crt.c.obj' failed

mingw32-make[2]: *** [library/CMakeFiles/mbedx509.dir/x509_crt.c.obj] Error 1

CMakeFiles\Makefile2:155: recipe for target 'library/CMakeFiles/mbedx509.dir/all' failed

mingw32-make[1]: *** [library/CMakeFiles/mbedx509.dir/all] Error 2

Makefile:139: recipe for target 'all' failed

mingw32-make: *** [all] Error 2

With ARMC5:

"x509_crt.c", line 90: Error:  #5: cannot open source input file "netinet/in.h": No such file or directory
  #include <netinet/in.h>
                         ^
x509_crt.c: 0 warnings, 1 error
make[1]: *** [x509_crt.o] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:166: recipe for target 'x509_crt.o' failed
  AR    libmbedcrypto.a
make: *** [lib] Error 2
Makefile:19: recipe for target 'lib' failed
^^^^build_armcc: build: ARM Compiler 5, make: command make CC=/usr/local/ARM_Compiler_5.06u3/bin//armcc AR=/usr/local/ARM_Compiler_5.06u3/bin//armar WARNING_CFLAGS=--strict --c99 lib -> 2^^^^

Have you tried building on Visual Studio as well?

I'm afraid your feature is not portable enough, and we may think of either doing a single function that implements the ip comparison, or have a different function for every c library I think that having a portable ip comparison should be preferred in this situation, and is more portable. @Patater Any thoughts on this?

[RonEld]

@ekoby Thank you for your change! Now, there is a failure only in Windows 2013 build:

     2>..\..\library\x509_crt.c(3009): error C2143: syntax error : missing ';' before 'type' [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]
     2>..\..\library\x509_crt.c(3010): error C2065: 'v' : undeclared identifier [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]
     2>..\..\library\x509_crt.c(3014): error C2065: 'v' : undeclared identifier [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]
     2>..\..\library\x509_crt.c(3015): error C2065: 'v' : undeclared identifier [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]
         Compiling...
         x509_csr.c
         x509write_crt.c
         x509write_csr.c
     2>Done Building Project "C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj" (Rebuild target(s)) -- FAILED.
     1>Done Building Project "C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.sln" (Rebuild target(s)) -- FAILED.

Build FAILED.

       "C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.sln" (Rebuild target) (1) ->
       "C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj" (Rebuild target) (2) ->
       (ClCompile target) -> 
         ..\..\library\x509_crt.c(3009): error C2143: syntax error : missing ';' before 'type' [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]
         ..\..\library\x509_crt.c(3010): error C2065: 'v' : undeclared identifier [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]
         ..\..\library\x509_crt.c(3014): error C2065: 'v' : undeclared identifier [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]
         ..\..\library\x509_crt.c(3015): error C2065: 'v' : undeclared identifier [C:\builds\ws\2906-merge-WGUMW4JEFVJ6HPM3NHFEM\worktrees\tmpu_iikssx\visualc\VS2010\mbedTLS.vcxproj]

    0 Warning(s)
    4 Error(s)

This is probably because the decleration of v is done in middle of code block.Please declare unsigned long v at beginning of code block. (line 3000, or even better, at beginning of function(line 2988) ). The assignment of v should remin in same location. It's strange why only this build and not all c99 builds failed though.

[RonEld]

The current CI failure is in the API\ABI checking script, not related to this PR:

<problems_with_types severity="Medium">

  <header name="crypto_struct.h">

    <type name="struct psa_key_derivation_s">

      <problem id="Removed_Field">

        <change target="can_output_key">Field @target has been removed from this type.</change>

        <effect>Applications will access incorrect memory when attempting to access this field.</effect>

      </problem>

      </type>

  </header>

</problems_with_types>



</report>

<report kind="source" version="1.2">



<problems_with_types severity="High">

  <header name="crypto_struct.h">

    <type name="struct psa_key_derivation_s">

      <problem id="Removed_Field">

        <change target="can_output_key">Field @target has been removed from this type.</change>

        <effect target="can_output_key" type_name="struct psa_key_derivation_s">Recompilation of a client program may be broken with the error message: '@type_name' has no member named '@target'.</effect>

      </problem>

      </type>

  </header>

</problems_with_types>

[ekoby]

@RonEld @Patater Do I need to do anything else for this PR? Thanks

[Patater]

CI passing on merge result. No backports required as this is a new capability.

Needs a ChangeLog entry to explain the feature and give credit to the contributor. @ekoby Please refer to the existing ChangeLog for examples and choose how you'd like to credit yourself.

[Patater]

Please rebase to remove the merge commit ("Merge remote-tracking branch 'upstream/development' into verify-ip-sa"). This makes your patch set easier to review and work with both now as well as into the future.

[ekoby]

Please rebase to remove the merge commit ("Merge remote-tracking branch 'upstream/development' into verify-ip-sa"). This makes your patch set easier to review and work with both now as well as into the future.

@Patater I've removed the merge commit and now I have conflicts in ChangeLog which I tried to avoid by merging first.

[ekoby]

what is holding this up?

[mpg]

@ekoby Another contributor has brought to our attention the fact that there's also a security issue with the way we currently (don't) handle IP SANs: https://github.com/ARMmbed/mbedtls/issues/3498 - so we've been developing, testing and reviewing a fix for the security part, which took priority over adding a new feature, which your PR is doing. The fix is nearly ready and we should be able to publish it in the next few days.

Once that's done, I'm afraid we're going to have to ask you to rebase your PR on top of the security fix, because they both touch some of the same lines of code, so there will be conflicts. However, you should find that the fix makes it easier to add support for handling IP SANs, with no risk of making existing tests fail - I've even left a comment in the code indicating where to add support for new types of SANs. (While rebasing, I suggest you move the ChangeLog entry from the monolithic ChangeLog file that's always causing conflicts to a new file in the ChangeLog.d directory that was designed to avoid those conflicts, see the Readme in that directory.)

After that, I'll make it a priority to review your PR. Unfortunately the original reviewers - Patater and RonEld - have move on to other roles in the meantime so they're unlikely to be able to review again, but I'm sure we'll be able to find a second reviewer to review your PR in a timely fashion after you rebased it.

Sorry for the delay and thanks for your patience and continued interest in contributing to Mbed TLS.

[ekoby]

@ekoby Another contributor has brought to our attention the fact that there's also a security issue with the way we currently (don't) handle IP SANs: #3498 - so we've been developing, testing and reviewing a fix for the security part, which took priority over adding a new feature, which your PR is doing. The fix is nearly ready and we should be able to publish it in the next few days.

Once that's done, I'm afraid we're going to have to ask you to rebase your PR on top of the security fix, because they both touch some of the same lines of code, so there will be conflicts. However, you should find that the fix makes it easier to add support for handling IP SANs, with no risk of making existing tests fail - I've even left a comment in the code indicating where to add support for new types of SANs. (While rebasing, I suggest you move the ChangeLog entry from the monolithic ChangeLog file that's always causing conflicts to a new file in the ChangeLog.d directory that was designed to avoid those conflicts, see the Readme in that directory.)

After that, I'll make it a priority to review your PR. Unfortunately the original reviewers - Patater and RonEld - have move on to other roles in the meantime so they're unlikely to be able to review again, but I'm sure we'll be able to find a second reviewer to review your PR in a timely fashion after you rebased it.

Sorry for the delay and thanks for your patience and continued interest in contributing to Mbed TLS.

Thanks, let me know when it's ready for rebase

[mpg]

Our fix for the security issue is in #3554. You'll probably find it more convenient to wait until we've merged it (which, if all goes well, could happen tomorrow) before you push a new, rebased version of your PR, but you're welcome to have a look now and perhaps even start reworking your PR locally - you're likely to want to insert some code here: https://github.com/ARMmbed/mbedtls/pull/3554/files#diff-7eb92ee9f705f88338240a484631c19cR3023

[mpg]

Note: #3554 has been merged, so everything is now ready for your to rebase this PR. While rebasing, feel free to either simplify the history or keep it as is, whatever's more convenient to you.

[ekoby]

addressed requested changes and added tests

[ekoby]

@mpg Any chance this PR can make it into the next release?

[mpg]

@ekoby Sorry for not coming back to you earlier. Unfortunately this PR missed the release of September the 1st, but considering we don't have a set date yet for our next release, I'd say the chances of your PR making it into the next release are very high!

I'm almost fully satisfied with the current state of the PR, the only remaining point is I think the newly-introduced parsing functions x509_parse_ipv4() and especially x509_parse_ipv6() should be unit-tested, including negative tests to exercise all the failure modes.

The current tests through x509_verify are good and should definitely be kept, but unit tests are better suited to more extensive negative testing.

I've been checking test coverage as follows:

make clean && (cd tests && make CFLAGS='--coverage -g3 -Og' test_suite_x509parse && ./test_suite_x509parse)
make lcov
firefox Coverage/library/x509_crt.c.gcov.html

then look for parse_ipv.

Currently these functions are static as they should be, which seems like it might make it hard to unit-test them, but we've recently added facilities for that, and since the latest release there are examples of MBEDTLS_STATIC_TESTABLE functions in our code base: mbedtls_ssl_cf_memcpy_offset() and mbedtls_ssl_cf_hmac() (in library/ssl_msg.c, declared in library/ssl_invasive.c, tested in tests/suite/test_suite_ssl.{function,data}.

Here's a summary of how to unit-test a function that should usually be static:

1. In the definition of the function, replace static with MBEDTLS_STATIC_TESTABLE (this expands to static unless MBEDTLS_TEST_HOOKS is defined.
2. Declare the function in a private header in the library folder (here you'll need to create library/x509_invasive.h), with the declaration guarded by #if defined(MBEDTLS_TEST_HOOKS)
3. Define a test function (in this case, one for IPv4, one for IPv6) in the suitable test suite, in your case tests/suites/test_suite_x509parse.function, and make it depend on MBEDTLS_TEST_HOOKS, that is the header should look like /* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */
4. Define test data normally in the corresponding data file. Those test will be skipped unless you uncomment MBEDTLS_TEST_HOOKS in include/mbedtls/config.h - so you'll want to uncomment that while developing your tests.

We want enough test data to exercise all success modes and most (preferably all) failure modes of each function.

I hope this is enough guidance for you to add those tests, but if you encounter any difficulty, feel free to ask questions.

[mpg]

A note on process: please don't mark the conversations as resolved, I'll do this myself when I review the updated PR - this makes it easier for me to ensure all my points have been addressed. (If you want to mark which points you've fixed, you mark the comment with the :rocket: emoji instead, but that's just if you find it helps.) Thanks!

[gstrauss]

@ekoby: I am not sure why this PR stalled out. Do you recall whether or not there was a requirement to implement local IPv4 and IPv6 parsing? Do you have thoughts on #6475 where I use inet_pton() instead? And thank you: I have cherry-picked some of the tests you added in this PR.

[ekoby]

@ekoby: I am not sure why this PR stalled out. Do you recall whether or not there was a requirement to implement local IPv4 and IPv6 parsing? Do you have thoughts on #6475 where I use inet_pton() instead? And thank you: I have cherry-picked some of the tests you added in this PR.

I believe it was stalled because of the reviewer churn. inet_pton() was used in initial implementation but is not suitable on all platforms: https://github.com/Mbed-TLS/mbedtls/pull/2906#issuecomment-546668319

[gstrauss]

@ekoby: I am not sure why this PR stalled out. Do you recall whether or not there was a requirement to implement local IPv4 and IPv6 parsing? Do you have thoughts on #6475 where I use inet_pton() instead? And thank you: I have cherry-picked some of the tests you added in this PR.

I believe it was stalled because of the reviewer churn. inet_pton() was used in initial implementation but is not suitable on all platforms: #2906 (comment)

Wow. It's been 3 years. I'm sorry you put in that effort for no tangible benefit (yet). I plan to be a bit more of a thorn in #6475 to try to make some progress and I am going to try to leverage some of the work you have done here. If you see something in #6475 taken from this PR and would like me to do it differently, please let me know. (code, commit attributions, commit messages, etc)

[gstrauss]

@ekoby I think that your implementation might allow excess characters at the end of an IPv4 address.

Besides that, strtoul() accepts a very wide variety of numerical syntaxes, including - and +, leading spaces, 0x for hex, and others. For stricter parsing of IP formats, I am going to write my own implementation which does not use strtoul()

[ekoby]

closing. letting @gstrauss carry the torch with https://github.com/Mbed-TLS/mbedtls/pull/6475

[gstrauss]

#6475 and #7436 have been merged into development branch for mbedtls-3.x