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

• Tests
• Documentation
• 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.

[Patater]

LGTM

We can resolve ChangeLog conflicts during the merge to development.

[RonEld]

I added a comment, which I don't think is a blocker by itself, but we are missing negative tests.

[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: #2906 (comment)

[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 #6475

[gstrauss]

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