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

Long certificate serial number with OpenSSL backend is null #235

Closed
dkjjr89 opened this Issue Apr 22, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@dkjjr89

dkjjr89 commented Apr 22, 2015

In lib/vtls/openssl/c in version 7.41.0 at line 2466 we have:

  ptr = bufp;
  *ptr++ = 0;
  if(num->type == V_ASN1_NEG_INTEGER)
    *ptr++='-';

  for(j=0; (j<num->length) && (left>=4); j++) {
    /* TODO: length restrictions */
    snprintf(ptr, 3, "%02x%c",num->data[j],
             ((j+1 == num->length)?'\n':':'));
    ptr += 3;
    left-=4;

Since bufp gets pushed to return a certificate serial number setting the first character to null will always cause null to be returned, therefore, line 2477 should be removed. The snprintf call attempts to create a colon separated string but just the hexadecimal value is being inserted. Also, I could not locate documentation that says the serial number should be colon separated. To get long serial numbers returned from the library I changed the above block to:

  ptr = bufp;
  if(num->type == V_ASN1_NEG_INTEGER)
    *ptr++='-';

  for(j=0; (j<num->length) && (left>=3); j++) {
    /* TODO: length restrictions */
    snprintf(ptr, 3, "%02x",num->data[j]);
    ptr += 2;
    left-=2;

Hope this helps!

@jay jay added the SSL/TLS label Apr 22, 2015

@bagder bagder self-assigned this Apr 26, 2015

bagder added a commit that referenced this issue Apr 26, 2015

openssl: fix serial number output
The code extracting the cert serial number was broken and didn't display
it properly.

Bug: #235
Reported-by: dkjjr89
@bagder

This comment has been minimized.

Member

bagder commented Apr 26, 2015

Thanks! I wrote up a slightly modified fix but based on your report and hints here. Landed in aff153f.

@bagder bagder closed this Apr 26, 2015

bagder added a commit that referenced this issue Apr 27, 2015

openssl: fix serial number output
The code extracting the cert serial number was broken and didn't display
it properly.

Bug: #235
Reported-by: dkjjr89
@dkjjr89

This comment has been minimized.

dkjjr89 commented Apr 27, 2015

Thank you!

@jay

This comment has been minimized.

Member

jay commented Apr 27, 2015

For other octets retrieved via CURLINFO_CERTINFO like rsa and signature a colon is used as the separator for each octet. I'm not sure why not for serial number. OpenSSL in their output uses the colon as a separator but only for long serial numbers (see openssl x509 -noout -text -in cert). I don't see why not do it that way for all. Though changing it to be consistent with the others at this point may break a user's parsing of it.

Another thing that looks strange in that area is output of negative serial numbers. The current way is to prefix the octets with - to designate negative direction (a la integer). but the way OpenSSL does it looks more correct.. although again any change at this point may break a user's parsing.

@bagder

This comment has been minimized.

Member

bagder commented Apr 27, 2015

@jay changing it could still be safe as it was completely broken before and thus was never parsed successfully anyway! I can see how matching openssl's output could be valuable.

I also glanced over the negative thing before I ignored it but you're right, we should make sure to output the same serial number that openssl does, even when negative.

@jay

This comment has been minimized.

Member

jay commented Apr 27, 2015

Ok. If you have no objections I'll replace that block with i2c_ASN1_INTEGER.

@bagder

This comment has been minimized.

Member

bagder commented Apr 27, 2015

No objections at all!

@jay

This comment has been minimized.

Member

jay commented Apr 27, 2015

They're not using i2c_ASN1_INTEGER, for the output. I assumed they were based on what I was reading. Mistake! I should've tested the output of a large negative serial number to be sure. I created a cert with a serial of -999,999,999,999,999,999,999:

openssl req -config openssl.cnf -x509 -newkey rsa:2048 -keyout serial_number_negative_nines.key -out serial_number_negative_nines.crt -days 3650 -nodes -batch -set_serial -999999999999999999999 -subj /CN=localhost/
openssl x509 -noout -text -in serial_number_negative_nines.crt

Here's the relevant part of their x509 output, which comes from X509_print_ex:

        Serial Number:
             (Negative)36:35:c9:ad:c5:de:9f:ff:ff

And if I specify -serial it also shows serial=-3635C9ADC5DE9FFFFF. So I guess there is some basis. A smaller number that fits in a long like -2000 shows Serial Number: -2000 (-0x7d0) and serial=-07D0.

What libcurl is doing right now is the same as the OpenSSL 'serial' format, not the OpenSSL 'Serial Number' format. That's probably fine given that nobody's used it yet, but if you want I can change it to their 'Serial Number' format as seen in X509_print_ex. libcurl had something similar to that for small numbers prior to your change but it would have to be modified to take into account negative numbers.

So it doesn't look like much of an issue anymore. Shame, the i2c method still looks more correct to me and easier to parse!

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue May 3, 2015

wiz
Update to 7.42.1:
Version 7.42.1 (28 Apr 2015)

Daniel Stenberg (28 Apr 2015)
- RELEASE-NOTES: 7.42.1 ready

- CURLOPT_HEADEROPT: default to separate

  Make the HTTP headers separated by default for improved security and
  reduced risk for information leakage.

  Bug: http://curl.haxx.se/docs/adv_20150429.html
  Reported-by: Yehezkel Horowitz, Oren Souroujon

- RELEASE-NOTES: synced with a6e0270e

- sws: init http2 state properly

  It would otherwise cause problems when running tests after 1801 etc.

- curl_easy_getinfo.3: document 'internals' in CURLINFO_TLS_SESSION

  ... as it was previouly undocumented what the pointer was.

- openssl: fix serial number output

  The code extracting the cert serial number was broken and didn't display
  it properly.

  Bug: curl/curl#235
  Reported-by: dkjjr89

- [Alessandro Ghedini brought this change]

  curl.1: fix typo

- RELEASE-NOTES: toward 7.42.1, synced with 097460a

- [Kamil Dudka brought this change]

  curl -z: do not write empty file on unmet condition

  This commit fixes a regression introduced in curl-7_41_0-186-g261a0fe.
  It also introduces a regression test 1424 based on tests 78 and 1423.

  Reported-by: Viktor Szakats
  Bug: curl/curl#237

- [Kamil Dudka brought this change]

  docs: distribute the CURLOPT_PINNEDPUBLICKEY(3) man page, too

- connectionexists: follow-up to fd9d3a1ef1f

  PROTOPT_CREDSPERREQUEST still needs to be checked even when NTLM is not
  enabled.

  Mistake-caught-by: Kamil Dudka

- connectionexists: fix build without NTLM

  Do not access NTLM-specific struct fields when built without NTLM
  enabled!

  bug: http://curl.haxx.se/?i=231
  Reported-by: Patrick Rapin

- dist: include {src,lib}/checksrc.whitelist

mamash pushed a commit to joyent/pkgsrc-legacy that referenced this issue Sep 18, 2015

wiz Filip Hajny
Update to 7.42.1:
Version 7.42.1 (28 Apr 2015)

Daniel Stenberg (28 Apr 2015)
- RELEASE-NOTES: 7.42.1 ready

- CURLOPT_HEADEROPT: default to separate

  Make the HTTP headers separated by default for improved security and
  reduced risk for information leakage.

  Bug: http://curl.haxx.se/docs/adv_20150429.html
  Reported-by: Yehezkel Horowitz, Oren Souroujon

- RELEASE-NOTES: synced with a6e0270e

- sws: init http2 state properly

  It would otherwise cause problems when running tests after 1801 etc.

- curl_easy_getinfo.3: document 'internals' in CURLINFO_TLS_SESSION

  ... as it was previouly undocumented what the pointer was.

- openssl: fix serial number output

  The code extracting the cert serial number was broken and didn't display
  it properly.

  Bug: curl/curl#235
  Reported-by: dkjjr89

- [Alessandro Ghedini brought this change]

  curl.1: fix typo

- RELEASE-NOTES: toward 7.42.1, synced with 097460a

- [Kamil Dudka brought this change]

  curl -z: do not write empty file on unmet condition

  This commit fixes a regression introduced in curl-7_41_0-186-g261a0fe.
  It also introduces a regression test 1424 based on tests 78 and 1423.

  Reported-by: Viktor Szakats
  Bug: curl/curl#237

- [Kamil Dudka brought this change]

  docs: distribute the CURLOPT_PINNEDPUBLICKEY(3) man page, too

- connectionexists: follow-up to fd9d3a1ef1f

  PROTOPT_CREDSPERREQUEST still needs to be checked even when NTLM is not
  enabled.

  Mistake-caught-by: Kamil Dudka

- connectionexists: fix build without NTLM

  Do not access NTLM-specific struct fields when built without NTLM
  enabled!

  bug: http://curl.haxx.se/?i=231
  Reported-by: Patrick Rapin

- dist: include {src,lib}/checksrc.whitelist

jgsogo added a commit to jgsogo/curl that referenced this issue Oct 19, 2015

openssl: fix serial number output
The code extracting the cert serial number was broken and didn't display
it properly.

Bug: curl#235
Reported-by: dkjjr89

aurimasc added a commit to Unity-Technologies/curl that referenced this issue Oct 30, 2015

openssl: fix serial number output
The code extracting the cert serial number was broken and didn't display
it properly.

Bug: curl#235
Reported-by: dkjjr89

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

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