Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenVPN cert chain with sha512 sigs and 4096-bit keys not correctly validated #31

Closed
binnacle opened this issue Sep 16, 2013 · 3 comments

Comments

@binnacle
Copy link

Have an OpenVPN configuration that works with
a WinXP client and Linux x86_64 server.

PolarSSL reject same exact certs that work with
aforementioned configuration.

Much detail at

https://community.openvpn.net/openvpn/ticket/332

@binnacle
Copy link
Author

2013-09-16 03:31:42 ----- OpenVPN Start -----
2013-09-16 03:31:42 EVENT: RESOLVE
2013-09-16 03:31:44 Contacting 74.95.187.106:1194 via TCP
2013-09-16 03:31:44 EVENT: WAIT
2013-09-16 03:31:44 Connecting to alt.flumedata.com:1194 (74.95.187.106) via TCPv4
2013-09-16 03:31:44 EVENT: CONNECTING
2013-09-16 03:31:44 Tunnel Options:V4,dev-type tun,link-mtu 1543,tun-mtu 1500,proto TCPv4_CLIENT,cipher BF-CBC,auth SHA1,keysize 128,key-method 2,tls-client
2013-09-16 03:31:44 Peer Info:
IV_VER=1.0
IV_PLAT=ios
IV_NCP=1

2013-09-16 03:31:45 VERIFY OK: depth=0
cert. version : 3
serial number : 07
issuer name  : C=US, ST=Nevada, L=Incline Village, O=Flume Data, Inc., OU=Certificate Authority, CN=Flume Data CA
subject name  : C=US, ST=Nevada, L=Incline Village, O=Flume Data, Inc., CN=alt.flumedata.com
issued  on    : 2012-10-05 22:53:41
expires on    : 2022-10-03 22:53:41
signed using  : RSA+SHA512
RSA key size  : 4096 bits

2013-09-16 03:31:45 VERIFY FAIL: depth=1
cert. version : 3
serial number : A9:02:96:B6:A1:7E:CD:A3
issuer name  : C=US, ST=Nevada, L=Incline Village, O=Flume Data, Inc., OU=Certificate Authority, CN=Flume Data CA
subject name  : C=US, ST=Nevada, L=Incline Village, O=Flume Data, Inc., OU=Certificate Authority, CN=Flume Data CA
issued  on    : 2012-10-05 22:51:57
expires on    : 2022-10-03 22:51:57
signed using  : RSA+SHA512
RSA key size  : 4096 bits

2013-09-16 03:31:45 Transport Error: PolarSSL: SSL read error : X509 - Certificate verification failed, e.g. CRL, CA or signature check failed
2013-09-16 03:31:45 EVENT: CERT_VERIFY_FAIL PolarSSL: SSL read error : X509 - Certificate verification failed, e.g. CRL, CA or signature check failed [ERR]
2013-09-16 03:31:45 EVENT: DISCONNECTED
2013-09-16 03:31:45 Raw stats on disconnect:
  BYTES_IN : 3892
  BYTES_OUT : 880
  PACKETS_IN : 29
  PACKETS_OUT : 35
  SSL_ERROR : 1
  CERT_VERIFY_FAIL : 1
2013-09-16 03:31:45 Performance stats on disconnect:
  CPU usage (microseconds): 97939
  Network bytes per CPU second: 48724
  Tunnel bytes per CPU second: 0
2013-09-16 03:31:45 ----- OpenVPN Stop -----
2013-09-16 03:31:45 EVENT: DISCONNECT_PENDING

@pjbakker
Copy link
Contributor

I receive a 404 on the attachments on OpenVPN Trac. Can you mail me the certificates: paul at polarssl dot org?

I'm going to check it for you..

@binnacle
Copy link
Author

Big Goof. Had old/stale version of CA cert lying around
and inadvertently used it in .ovpn config. Works find
with correct CA. Apologies for erroneous report.

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Sep 5, 2017
mpg added a commit that referenced this issue Sep 2, 2020
The previous code used comparison operators >= and == that are quite likely to
be compiled to branches by some compilers on some architectures (with some
optimisation levels).

For example, take the following function:

void old_update( size_t data_len, size_t *padlen )
{
    *padlen  *= ( data_len >= *padlen + 1 );
}

With Clang 3.8, let's compile it for the Arm v6-M architecture:

% clang --target=arm-none-eabi -march=armv6-m -Os foo.c -S -o - |
    sed -n '/^old_update:$/,/\.size/p'

old_update:
        .fnstart
@ BB#0:
        .save   {r4, lr}
        push    {r4, lr}
        ldr     r2, [r1]
        adds    r4, r2, #1
        movs    r3, #0
        cmp     r4, r0
        bls     .LBB0_2
@ BB#1:
        mov     r2, r3
.LBB0_2:
        str     r2, [r1]
        pop     {r4, pc}
.Lfunc_end0:
        .size   old_update, .Lfunc_end0-old_update

We can see an unbalanced secret-dependant branch, resulting in a total
execution time depends on the value of the secret (here padlen) in a
straightforward way.

The new version, based on bit operations, doesn't have this issue:

new_update:
        .fnstart
@ BB#0:
        ldr     r2, [r1]
        subs    r0, r0, #1
        subs    r0, r0, r2
        asrs    r0, r0, #31
        bics    r2, r0
        str     r2, [r1]
        bx      lr
.Lfunc_end1:
        .size   new_update, .Lfunc_end1-new_update

(As a bonus, it's smaller and uses less stack.)

While there's no formal guarantee that the version based on bit operations in
C won't be translated using branches by the compiler, experiments tend to show
that's the case [1], and it is commonly accepted knowledge in the practical
crypto community that if we want to sick to C, bit operations are the safest
bet [2].

[1] https://github.com/mpg/ct/blob/master/results
[2] https://github.com/veorq/cryptocoding
mpg added a commit that referenced this issue Sep 18, 2020
The previous code used comparison operators >= and == that are quite likely to
be compiled to branches by some compilers on some architectures (with some
optimisation levels).

For example, take the following function:

void old_update( size_t data_len, size_t *padlen )
{
    *padlen  *= ( data_len >= *padlen + 1 );
}

With Clang 3.8, let's compile it for the Arm v6-M architecture:

% clang --target=arm-none-eabi -march=armv6-m -Os foo.c -S -o - |
    sed -n '/^old_update:$/,/\.size/p'

old_update:
        .fnstart
@ BB#0:
        .save   {r4, lr}
        push    {r4, lr}
        ldr     r2, [r1]
        adds    r4, r2, #1
        movs    r3, #0
        cmp     r4, r0
        bls     .LBB0_2
@ BB#1:
        mov     r2, r3
.LBB0_2:
        str     r2, [r1]
        pop     {r4, pc}
.Lfunc_end0:
        .size   old_update, .Lfunc_end0-old_update

We can see an unbalanced secret-dependant branch, resulting in a total
execution time depends on the value of the secret (here padlen) in a
straightforward way.

The new version, based on bit operations, doesn't have this issue:

new_update:
        .fnstart
@ BB#0:
        ldr     r2, [r1]
        subs    r0, r0, #1
        subs    r0, r0, r2
        asrs    r0, r0, #31
        bics    r2, r0
        str     r2, [r1]
        bx      lr
.Lfunc_end1:
        .size   new_update, .Lfunc_end1-new_update

(As a bonus, it's smaller and uses less stack.)

While there's no formal guarantee that the version based on bit operations in
C won't be translated using branches by the compiler, experiments tend to show
that's the case [1], and it is commonly accepted knowledge in the practical
crypto community that if we want to sick to C, bit operations are the safest
bet [2].

[1] https://github.com/mpg/ct/blob/master/results
[2] https://github.com/veorq/cryptocoding

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
gilles-peskine-arm pushed a commit that referenced this issue Apr 21, 2021
The previous code used comparison operators >= and == that are quite likely to
be compiled to branches by some compilers on some architectures (with some
optimisation levels).

For example, take the following function:

void old_update( size_t data_len, size_t *padlen )
{
    *padlen  *= ( data_len >= *padlen + 1 );
}

With Clang 3.8, let's compile it for the Arm v6-M architecture:

% clang --target=arm-none-eabi -march=armv6-m -Os foo.c -S -o - |
    sed -n '/^old_update:$/,/\.size/p'

old_update:
        .fnstart
@ BB#0:
        .save   {r4, lr}
        push    {r4, lr}
        ldr     r2, [r1]
        adds    r4, r2, #1
        movs    r3, #0
        cmp     r4, r0
        bls     .LBB0_2
@ BB#1:
        mov     r2, r3
.LBB0_2:
        str     r2, [r1]
        pop     {r4, pc}
.Lfunc_end0:
        .size   old_update, .Lfunc_end0-old_update

We can see an unbalanced secret-dependant branch, resulting in a total
execution time depends on the value of the secret (here padlen) in a
straightforward way.

The new version, based on bit operations, doesn't have this issue:

new_update:
        .fnstart
@ BB#0:
        ldr     r2, [r1]
        subs    r0, r0, #1
        subs    r0, r0, r2
        asrs    r0, r0, #31
        bics    r2, r0
        str     r2, [r1]
        bx      lr
.Lfunc_end1:
        .size   new_update, .Lfunc_end1-new_update

(As a bonus, it's smaller and uses less stack.)

While there's no formal guarantee that the version based on bit operations in
C won't be translated using branches by the compiler, experiments tend to show
that's the case [1], and it is commonly accepted knowledge in the practical
crypto community that if we want to sick to C, bit operations are the safest
bet [2].

[1] https://github.com/mpg/ct/blob/master/results
[2] https://github.com/veorq/cryptocoding

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
SebastianBoe pushed a commit to SebastianBoe/mbedtls-1 that referenced this issue Jan 26, 2022
Apply Zephyr-specific changes to upstream
iameli pushed a commit to livepeer/mbedtls that referenced this issue Dec 5, 2023
Correct intermittent failure in stat_driver on sparc 64 target
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants