search

crocs-muni / usable-cert-validation

Research initiative to make TLS certificate validation usable.
https://x509errors.org
MIT License
19 stars 3 forks source link

Indicate deprecated and unused OpenSSL errors #48

Closed ericvalcik closed 4 years ago

ericvalcik commented 4 years ago

Hi,

Here is my proposal to fix Issue #24, feel free to tell me what to improve.

I separated the X509_V_ERR_OUT_OF_MEM into a commit of its own (1441c3e) because I'm not really sure if this error qualifies as unused.

ericvalcik commented 4 years ago

Thank you for the feedback. I fixed all the issues mentioned above. Also, I'm now aware that I labeled these error codes without much thought. So I did a little research and divided the 8 codes into 3 groups, each group has its own commit.

X509_V_ERR_OUT_OF_MEMORY, X509_V_ERR_UNSPECIFIED

X509_V_ERR_APPLICATION_VERIFICATION, __X509_V_ERR_UNABLE_TO_DECRYPT_CRL_SIGNATURE__

X509_V_ERR_AKID_SKID_MISMATCH, X509_V_ERR_AKID_ISSUER_SERIAL_MISMATCH, X509_V_ERR_SUBJECT_ISSUER_MISMATCH, __X509_V_ERR_KEYUSAGE_NO_CERTSIGN__

zacikpa commented 4 years ago

X509_V_ERR_OUT_OF_MEMORY, X509_V_ERR_UNSPECIFIED

  • On second thought, these codes are not deprecated or unused in my opinion, so I removed the openssl-unused tag.

Indeed, these two are neither deprecated nor unused. I would leave them as they are (only uncategorized) since they have nothing to do with the actual path validation.

X509_V_ERR_APPLICATION_VERIFICATION, X509_V_ERR_UNABLE_TO_DECRYPT_CRL_SIGNATURE

  • These codes are mentioned in the code but are not returned (I could be wrong). So I've left them labeled as unused (deprecated).

X509_V_ERR_APPLICATION_VERIFICATION This is a very specific error that applications using the openssl API might return. I understand its meaning like this: While the library cannot produce it by itself, it is the recommended error to use when something else in your application fails and you want to return an openssl-compatible error. I would leave this one without the unused tag.

X509_V_ERR_UNABLE_TO_DECRYPT_CRL_SIGNATURE This one truly only has a definition in the code, the unused tag suits it well.

X509_V_ERR_AKID_SKID_MISMATCH, X509_V_ERR_AKID_ISSUER_SERIAL_MISMATCH, X509_V_ERR_SUBJECT_ISSUER_MISMATCH, X509_V_ERR_KEYUSAGE_NO_CERTSIGN

  • These codes are not only mentioned in the code but also returned by some functions. I did some tracking by hand and I believe that these codes cannot be returned all the way to the user. (again, I could be easily wrong)
  • These codes can be returned all the way to a function check_issued in x509_vfy.c, but this function only returns 1 or 0, so the error codes won't be propagated deeper. So again, I left them labeled as unused.

Good job tracking that down. These four are also marked deprecated in the official docs, so your assumptions are correct.

zacikpa commented 4 years ago

There are still at least three errors that I would personally tag unused. If you are interested, take a deeper look at:

Though I am pretty sure that we can mark these three unused right away.

I am not really sure what to do with these:

But I assume they might be returned by higher-level applications in the same way as X_509_V_ERR_APPLICATION_VERIFICATION, so I wouldn't personally tag them.

Thank you for spending your time on this.

ericvalcik commented 4 years ago

I'll take a look at those three you mentioned and add them to the rest. I will also remove the label openssl-unused from X509_V_ERR_APPLICATION_VERIFICATION.

Thank you for your input.

ericvalcik commented 4 years ago

I now pushed the last changes I wanted to make.

I removed the openssl-unused tag from X509_V_ERR_APPLICATION_VERIFICATION and added it to X509_V_ERR_UNSUPPORTED_EXTENSION_FEATURE and X509_V_ERR_UNSUPPORTED_CONSTRAINT_SYNTAX.

I already had error code __X509_V_ERR_UNABLE_TO_DECRYPT_CRL_SIGNATURE__ tagged as unused or deprecated, so I left it as is.

ericvalcik commented 4 years ago

I've made the requested changed.

I also added two new codes as deprecated/unused: __X509_V_ERR_PROXY_SUBJECT_INVALID and X509_V_ERR_PATH_LOOP__

__X509_V_ERR_PROXY_SUBJECT_INVALID is not even defined in the code. X509_V_ERR_PATH_LOOP__ - I wanted to replicate this code, but I failed. I believe that this code cannot be returned from OpenSSL. It is only used in function check_issued in x509_vfy.c, but this function returns only 1 or 0. Also, there is no way of propagating this error outside this function (e.g. via pointers). It is only assigned to variable ret and this variable ceases to exist outside of this function.

ericvalcik commented 4 years ago

Ah, in the last commit I changed the sed command in Makefile in the __X509_V_ERR_CERT_SIGNATURE_FAILURE__ folder in hopes of fixing why the Travis check failed. Travis passed on my repo on another branch, but here it failed again. I can change the changed sed command to the one that was there before.

I used the sed command to not match the pattern only on single lines but to always add the following line to the current compared line and after that try to match the patter. That way I thought I could identify the last line of the certificate and change the last couple of chars to make the signature invalid. I found this solution here and modified it a bit.

zacikpa commented 4 years ago

In the not yet merged #66, I've replaced the signature failure certificate with a static one, so that it doesn't cause any issues in the future. If you come up with a better solution, feel free to change it, but I'd suggest making a separate PR for that.

zacikpa commented 4 years ago

@zacikpa: Should I just merge it anyway and you'll rebase #66 on the new master and rework it into a static chain for the CI to pass?

I'm okay with that.