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.

Sign up for GitHub

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

Jump to bottom

Pkcs7 #1

Open
wants to merge 15 commits into
base: development-pkcs7
Choose a base branch
from
Open

Pkcs7 #1

wants to merge 15 commits into from

Conversation

daxtens
Copy link

@daxtens daxtens commented May 29, 2020

Hi Nayna,

As discussed, here are my proposed changes for your PKCS#7 code.

My one big outstanding concern is described in the pkcs7: make certificate optional patch:

I'm a bit worried that we don't check the OIDs here or in many places
in the code, I think we could end up trying to parse the wrong things:
for example if there's a CRL but no certificates.

I think the code just assumes that things will be laid out as expected and I'm not sure that's a valid assumption?

The rest of it is mostly fairly straightforward and should be all in neat little commits.

The final 3 commits need some brief explanation:

  • We don't want to print on success in the final product. I think maybe we don't want to print on failure either but instead make sure mbedtls_strerror works as it should. But I'm not sure.
  • The final patch isn't for pkcs7, it's a patch I've been using for one of my integrations, you can pick it up and submit it if you like, or not - entirely up to you.

Nayna Jain and others added 14 commits May 20, 2020 17:41
add support for pkcs7
64daa2e 
PKCS7 signing format is used by OpenPOWER Key Management, which is
using mbedtls as its crypto library.

This patch adds the limited support of pkcs7 parser and verification
to the mbedtls. The limitations are:

* Only signed data is supported.
* No CRLs are handled.
* No optionals are handled.
* Single signer is supported.
* Only SHA256 as the digest algorithm is supported.

Signed-off-by: Nayna Jain <[email protected]>
@daxtens
pkcs7: build under CMake
ac742c6 
Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: only include file I/O if permitted by configuration
277e050 
Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: create sha1 test file
8529415 
it was empty

openssl cms -sign -in data_files/pkcs7.data -signer data_files/cli-rsa-sha256.crt \
            -inkey data_files/cli-rsa-sha256.key.der  -keyform der -outform der \
            -binary -nosmimecap -md sha1 -noattr -out data_files/pkcs7.data.signed.sha1

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: formatting: convert tabstop to 4 spaces
e5d66f0 
Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: tests: standardise on no space after depends_on:
801cfd6 
Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: tests: indentation - (hopefully) makes a Travis CI test happy
40fc2ab 
See https://travis-ci.com/github/daxtens/mbedtls/builds/168526337

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: tests: prevent crash if parse fails
93b020d 
If the pkcs7 der doesn't parse, the test will free the x509 cert too soon,
causing memory corruption and crashes.

Likewise in the fail tests, things going not as expected can lead to things
breaking.

Picked up by compiling with clang.

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: make certificate optional
f7eb29a 
The signatures from sign-file do not containe the embedded certificate.

I'm a bit worried that we don't check the OIDs here or in many places
in the code, I think we could end up trying to parse the wrong things:
for example if there's a CRL but no certificates.

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: support any recognised MD type
e8ee269 
mbedtls provides some support functions that make this really easy

I'm not sure if it's desirable to provide a way to support only certain
algorithms, I think maybe if the aim is just to not support weak algos
like SHA-1 then just disabling SHA-1 in the config file would suffice.

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: provide fuzz harness
1812ffb 
This allows for fuzz testing with OSS-Fuzz.

There is (bad, but sort-of adequate) documentation in programs/fuzz/README.md

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: use OID_CMP macro
859df88 
Not only is this easier to read, it avoids to avoid overreading
when oid.len > len(OID CONSTANT).

This was picked up with the fuzzer.

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: do not print info on success
899aed0 
Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
pkcs7: ??? don't print on error
d54118b 
I'm not sure if it makes sense to print lots of output for errors.
It certainly makes fuzzing difficult.

Signed-off-by: Daniel Axtens <[email protected]>
@naynajain
Copy link
Owner

Hi Nayna,

As discussed, here are my proposed changes for your PKCS#7 code.

Thanks Daniel !! Thanks for adding I/O, fuzz, CMake, OID comparision related changes.

My one big outstanding concern is described in the pkcs7: make certificate optional patch:

I'm a bit worried that we don't check the OIDs here or in many places
in the code, I think we could end up trying to parse the wrong things:
for example if there's a CRL but no certificates.

I am not sure if I am very clear. If someone adds a CRL but no certificate and if code is assuming it to be a certificate, won't call to pkcs7_get_certificates() which further calls mbedtls_x509_crt_parse() fail in parsing ?

I think the code just assumes that things will be laid out as expected and I'm not sure that's a valid assumption?

The rest of it is mostly fairly straightforward and should be all in neat little commits.

The final 3 commits need some brief explanation:

Yeah.. and some needs clean up of description. As I merge them in my branch, commit descriptions would appear as they are just now. Would you be ok to fix them ? Or you are expecting if I can fix those ?

  • We don't want to print on success in the final product. I think maybe we don't want to print on failure either but instead make sure mbedtls_strerror works as it should. But I'm not sure.

I think its ok if we do not add on success, but for error I think it depends on choice. Code wise I do see print on errors in other code also in the mbedtls and personally I feel it helps in pointing exact reason. We can remove now and add over the time if we really feel that it would be better to have error codes. So, as of now its ok to remove it. I think both the patches that clean up print (error and success case) can be merged as single one.

  • The final patch isn't for pkcs7, it's a patch I've been using for one of my integrations, you can pick it up and submit it if you like, or not - entirely up to you.

I am not sure exactly which one it is. The first patch is I/O based and is needed. The last patch clears the error prints which is also needed . Can you tell with subject line ?

Overall,

I think:

  1. Need to fix commit descriptions
  2. Two print clean up patches can be merged
  3. mbedtls: x509: no time . I am not sure whether the problem was in general, or because of pkcs7 code ? And if it is in general, then can you put it last in the sequence with good patch description.

Thanks & Regards,
- Nayna

@daxtens
Copy link
Author

daxtens commented Jun 10, 2020

Hi Nayna,

Thanks!

I am not sure if I am very clear. If someone adds a CRL but no certificate and if code is assuming it to be a certificate, won't call to pkcs7_get_certificates() which further calls mbedtls_x509_crt_parse() fail in parsing ?

I think you're right. What I was wondering is if we should detect that it's a CRL before we get to the mbedtls_x509_crt_parse() function?

I think its ok if we do not add on success, but for error I think it depends on choice. Code wise I do see print on errors in other code also in the mbedtls and personally I feel it helps in pointing exact reason.

I am happy to leave the choice about whether to print on error up to you. The only real downside is slightly slower fuzzing. If we want to leave the prints in, we could maybe put them inside #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION blocks? This is becoming the standard way to disable code in fuzzing: see https://llvm.org/docs/LibFuzzer.html and Google's OSS-Fuzz.

With commit messages: I was thinking you might want to squash all or some of these patches into your base patch. I was thinking you'd squash everything except the x509: no time patch and maybe the fuzz harness patch. I wasn't expecting you to keep the commit descriptions in the final upstream submission - they were just intended for you to be able to easily understand what I was trying to do so you could be confident in squashing the patches in to yours.

I am not sure exactly which one it is. The first patch is I/O based and is needed. The last patch clears the error prints which is also needed . Can you tell with subject line ?

The patch I was trying to refer to was mbedtls: x509: no time, sorry it didn't end up actually being the last patch in the series!!

You mentioned elsewhere that you were having issues with the cmake build - I'm happy to have a look if you post your error message and compiler version.

@daxtens
Copy link
Author

daxtens commented Jun 10, 2020 •
edited
Loading

  1. mbedtls: x509: no time . I am not sure whether the problem was in general, or because of pkcs7 code ? And if it is in general, then can you put it last in the sequence with good patch description.

In general, not related to pkcs7. I will add a better description.

@daxtens
mbedtls: x509: do not include time.h without MBEDTLS_HAVE_TIME
b2e0c94 
MBEDTLS_HAVE_TIME is documented as: "System has time.h and time()."

If that is not defined, do not attempt to include time.h.

Signed-off-by: Daniel Axtens <[email protected]>
@daxtens
Copy link
Author

daxtens commented Jun 11, 2020

Updated x509 time commit.

@naynajain
Copy link
Owner

Hi Daniel,

You mentioned elsewhere that you were having issues with the cmake build - I'm happy to have a look if you post your error message and compiler version.

Yes.. Thanks.. Here are the details..

My gcc version is - gcc version 9.3.0 (Ubuntu 9.3.0-10ubuntu2)
The error is -
/home/linux/mbedtls/mbedtls/programs/ssl/ssl_context_info.c: In function ‘read_next_b64_code’:
/home/linux/mbedtls/mbedtls/programs/ssl/ssl_context_info.c:384:16: error: comparison is always true due to limited range of data type [-Werror=type-limits]
384 | while( EOF != c )

Still looking at your other comments...

@naynajain naynajain force-pushed the development-pkcs7 branch 2 times, most recently from 2e52cf2 to 05b7267 Compare June 13, 2020 02:00
daxtens pushed a commit to daxtens/mbedtls-1 that referenced this pull request Nov 25, 2020
@mpg
Use bit operations for constant-flow padding check
2ddec43 
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, naynajain#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, naynajain#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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daxtens @naynajain