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

clang-tidy target and fixes #3163

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@bagder
Member

bagder commented Oct 24, 2018

This adds a 'tidy' make target in the lib directory and two associated cleanup commits that address tidy warnings.

There is still a tidy warning left for @monnerat to ponder on: the left shift at the end of int2str() in x509asn1.c is done on a signed variable, and this isn't a defined behavior so we should either make it unsigned or not use shift...

Also @monnerat, I added lots of return code checks to x509asn1.c in this PR that fixes tidy warnings but I could certainly use an extra set of eyes to check that I did mess those up too bad!

@monnerat

This comment has been minimized.

Collaborator

monnerat commented Oct 24, 2018

I initially omited these success/failure checks on purpose, because the parsed certificate format has already been validated by some SSL backend and these procedures are not intended to be used for certificate format validation. So these new checks should never fail... unless a code bug! As comment at x509asn1.c:98 says, there is no intention here in rewriting an SSL library, thus I kept it as small and simple as possible.

As a consequence, the goal of all these success/failure tests is only to silent the tidy warnings, but will grow the code footstep uselessly.

As you know, I'm not anymore in a position to test all changes to x509asn1.c, but they seem OK to me after an eye check.

There is still a tidy warning left to ponder on: the left shift at the end of int2str() in x509asn1.c is done on a signed variable...

Do I push a fix on master or can I push to branch origin/bagder/clang-tidy ?

@bagder

This comment has been minimized.

Member

bagder commented Oct 26, 2018

the goal of all these success/failure tests is only to silent the tidy warnings, but will grow the code footstep uselessly.

The question is: can we be really sure of that? If we really can be sure of that, then I'd probably instead suggest that we provide a macro or second function that explicitly uses void to explicitly not return success/error for those places in the code where we currently skip the check.

Ultimately, I want to enable 'clang-tidy' runs in the CI and elsewhere so we need to fix the warnings, and this appears to be a warning that I'd rather not just disable globally (which I've done with a few others that just don't work out for us, as can be seen in this PR).

I'm not anymore in a position to test all changes to x509asn1.c

We really should create unit tests for this function so that it'd be tested by the CI.

Do I push a fix on master or can I push to branch origin/bagder/clang-tidy ?

Until we've decided on how to handle the return codes I think it is easier and has a lesser conflict-risk if you fix that independently of this branch.

I think I'll merge the gtls.c fix independently too...

@bagder

This comment has been minimized.

Member

bagder commented Oct 26, 2018

For the record: the list of clang-tidy warnings my clang-tidy-8 shows right now on git master.

@monnerat

This comment has been minimized.

Collaborator

monnerat commented Oct 26, 2018

The question is: can we be really sure of that?

At least, this is the intention: currently, this code is only called after a successful parse from an SSL library. Failures can only occur on a parse error (there's no allocation in this code), and could only be explained by a bug, either locally or in the SSL library.

If we really can be sure of that, then I'd probably instead suggest that we provide a macro or second function that explicitly uses void to explicitly not return success/error for those places in the code where we currently skip the check.

Alternatively, we could use an exception-like mechanism based on longjmp(), but this function is not yet used in libcurl. This would save a lot of tests at the cost of some additional storage for each handle, limit the footstep growth, and could even be used elsewhere in libcurl. If you are interested in this solution, I can develop macros for it.

@bagder

This comment has been minimized.

Member

bagder commented Oct 26, 2018

Failures can only occur on a parse error (there's no allocation in this code), and could only be explained by a bug, either locally or in the SSL library.

If there's a bug in this code, a malicious HTTPS server that knows the client is curl could possibly craft a response to trigger that particular bug. The SSL library that handles the TLS handshake and everything is not going to police and correctness-check the entire thing before we get a chance to parse the cert. So there's a risk that there's "crafted crap" coming in here and it is important that our parser can handle almost any junk passed in without doing anything bad.

Alternatively, we could use an exception-like mechanism based on longjmp

I'm not a fan of such custom solutions. They make the code look unusual and is harder for random developers to read and understand. I much prefer regular error checks plus returns.

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Oct 26, 2018

bagder added some commits Oct 22, 2018

test1651: unit test Curl_extract_certinfo()
The custom version used for Gskit, NSS, GnuTLS, WolfSSL and schannel.

Currently hangs!
@bagder

This comment has been minimized.

Member

bagder commented Oct 26, 2018

Here I'm introducing test case 1651 that verifies the Curl_extract_certinfo() function (if the function is built, which depends on what TLS backend that's used) - and with only a very basic "fuzzing" (writing byte 1 to a few different indexes) I could immediately trigger an infinite loop in the original code but the version in this branch "succeeds" (by succeeding I mean that it doesn't get stuck).

@monnerat

This comment has been minimized.

Collaborator

monnerat commented Oct 26, 2018

Here I'm introducing test case 1651 ... I could immediately trigger an infinite loop in the original code ...

This test calls Curl_extract_certinfo() without a previous validation by the SSL library, which is the exact thing that should not be done: certificates passed to x509asn1.c procedures are supposed to be pre-validated by the SSL library. Again, see comment at x509asn1.c:99.

I'll let you decide if you want to introduce these checks anyway.

I'll push a fix for the signed left shift soon.

@bagder

This comment has been minimized.

Member

bagder commented Oct 27, 2018

certificates passed to x509asn1.c procedures are supposed to be pre-validated by the SSL library

I know it says so and I know that has been the assumption so far. I'm suggesting that we can and should do better:

  1. We don't know exactly what level of pre-validation that is done by these different SSL libraries and I'm convinced that they all don't validate the entire certificate and all its fields. The question is then which fields they left unvalidated for us. I assume the exact set will differ depending on SSL library.
  2. Assuming correct input is a trap that makes these functions almost impossible to test/fuzz which then also makes it very hard for us to discovered "normal" bugs on valid input. We don't know exactly what "validated" means, as we have different parsers and thus different levels of strictness and different error handling logic.

In other words: I think we must make these functions able to survive crap/malicious input.

monnerat added a commit that referenced this pull request Oct 27, 2018

x509asn1: suppress left shift on signed value
Use an unsigned variable: as the signed operation behavior is undefined,
this change silents clang-tidy about it.

Ref: #3163
Reported-By: Daniel Stenberg

@bagder bagder closed this in be20814 Oct 27, 2018

@bagder bagder deleted the bagder/clang-tidy branch Oct 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment