Enlarge _oid2txt buffer to handle larger OIDs by frasertweedale · Pull Request #3612 · pyca/cryptography

frasertweedale · 2017-05-26T02:53:12Z

The OpenSSL manual recommends a buffer size of 80 for OBJ_oid2txt:
https://www.openssl.org/docs/crypto/OBJ_nid2ln.html#return_values.
But OIDs longer than this occur in real life (e.g. Active Directory
makes some very long OIDs). Use a more conservative buffer size of
256 to prevent crashes if the length of the stringified-OID exceeds
80 characters.

mention-bot · 2017-05-26T02:53:14Z

@frasertweedale, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alex, @reaperhulk and @intgr to be potential reviewers.

alex · 2017-05-26T02:55:49Z

It looks like you forgot to git add bigoid.pem.

reaperhulk · 2017-05-26T02:57:09Z

bigoid.pem will also need to be documented in the test-vectors.rst when it's added 😄

frasertweedale · 2017-05-26T02:58:23Z

@reaperhulk whups, thanks! test-vectors.rst update imminent.

alex · 2017-05-26T03:38:11Z

@stevepeak we appear to have hit a bug of some sort - the Changes page for this diff isn't showing the actual source, just a blank space:

stevepeak · 2017-05-26T14:15:32Z

Cool :) Nice edge case. I'll review 👍

alex · 2017-05-26T19:24:26Z

@stevepeak thanks, we appreciate it!

frasertweedale · 2017-05-29T02:09:19Z

Don't merge this; better fix coming.

The OpenSSL manual recommends a buffer size of 80 for OBJ_oid2txt: https://www.openssl.org/docs/crypto/OBJ_nid2ln.html#return_values. But OIDs longer than this occur in real life (e.g. Active Directory makes some very long OIDs). If the length of the stringified OID exceeds the buffer size, allocate a new buffer that is big enough to hold the stringified OID, and re-do the conversion into the new buffer.

alex · 2017-05-29T02:18:21Z

Is it possible to call OBJ_obj2txt with a NULL buffer to get the size?

frasertweedale · 2017-05-29T02:34:28Z

On Sun, May 28, 2017 at 07:18:22PM -0700, Alex Gaynor wrote: Is it possible to call `OBJ_obj2txt` with a `NULL` buffer to get the size?

This works. The downside is that we compute the conversion for the whole OID twice, every single time. With the first run using an 80-byte buffer we compute the conversion only once in the overwhelming majority of cases. Your call.

alex · 2017-05-29T02:37:06Z

I feel like the correct answer is "let's benchmark!", but that seems like way too much work to ask you to do for what should be simple. @reaperhulk or I will review as-is I think.

tiran · 2017-05-29T06:37:08Z

+    # alloc a big-enough buffer and go again.
    res = backend._lib.OBJ_obj2txt(buf, buf_len, obj, 1)
+    if res > buf_len - 1:  # account for terminating null byte
+        buf_len = res + 1


The fix is not sufficient. OBJ_obj2txt(buf, buf_len, obj, 1) does not return the actual required buffer size. You have to call the function with NULL buffer to make it calculate the correct size: OBJ_obj2txt(NULL, 0, obj, 1).

(This is not accurate, a later tiran comment shows that it does return required buffer size)

frasertweedale · 2017-05-29T06:46:22Z

    buf_len = 80
    buf = backend._ffi.new("char[]", buf_len)
+
+    # 'res' is the number of bytes that *would* be written if the


This is maybe not correct. More testing needed. Don't merge.

(Confirmed that this is correct in a later comment by tiran)

tiran · 2017-05-29T08:25:09Z

Your code was correct. It looks like I screwed up while I was adopting your patch for CPython. The ssl module calls OBJ_obj2txt() in two different functions. The second time it calls the function with no_name=1. I think I mistakenly copied code with no_name=0.

test case

#include <stdio.h>
#include "openssl/x509.h"

int main(int argc, char **argv) {
    char buf[5];
    char largebuf[256];
    int buflen, i;
    ASN1_OBJECT *obj;
    obj = OBJ_nid2obj(NID_server_auth);

    for (i=0; i < 2; i++) {
        buflen = OBJ_obj2txt(NULL, 0, obj, i);
        fprintf(stdout, "NULL buffer\t\tno_name: %d\tbuflen: %d\n", i, buflen);
        buflen = OBJ_obj2txt(buf, sizeof(buf), obj, i);
        fprintf(stdout, "small buffer %d\t\tno_name: %d\tbuflen: %d\n",
                sizeof(buf), i, buflen);
        buflen = OBJ_obj2txt(largebuf, sizeof(largebuf), obj, i);
        fprintf(stdout, "large buffer %d\tno_name: %d\tbuflen: %d\n",
                sizeof(largebuf), i, buflen);
        fprintf(stdout, "%s\n", largebuf);
    }
}

output

$ gcc -lcrypto obj2txt.c -o obj2txt && ./obj2txt 
NULL buffer             no_name: 0      buflen: 29
small buffer 5          no_name: 0      buflen: 29
large buffer 256        no_name: 0      buflen: 29
TLS Web Server Authentication
NULL buffer             no_name: 1      buflen: 17
small buffer 5          no_name: 1      buflen: 17
large buffer 256        no_name: 1      buflen: 17
1.3.6.1.5.5.7.3.1

frasertweedale · 2017-05-29T11:27:35Z

@tiran thanks for the update 👍

reaperhulk

this approach is fine by me

reaperhulk · 2017-05-29T21:32:42Z

@alex if you have no objections I'll merge this shortly so it can be in 1.9.

alex · 2017-05-29T21:33:05Z

I haven't reviewed, but if you've reviewed I'm sure it's fine.

…

On Mon, May 29, 2017 at 5:32 PM, Paul Kehrer ***@***.***> wrote: @alex <https://github.com/alex> if you have no objections I'll merge this shortly so it can be in 1.9. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3612 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAADBMeRupZbA_CbhBXPUkiptyAntdDLks5r-zl7gaJpZM4NnHdW> .

-- "I disapprove of what you say, but I will defend to the death your right to say it." -- Evelyn Beatrice Hall (summarizing Voltaire) "The people's good is the highest law." -- Cicero GPG Key fingerprint: D1B3 ADC0 E023 8CA6

alex added the x509 label May 26, 2017

alex added this to the Nineteenth Release milestone May 26, 2017

frasertweedale force-pushed the fix/long-oid-parse-failure branch from 5101e7a to f3f77f9 Compare May 26, 2017 02:57

frasertweedale force-pushed the fix/long-oid-parse-failure branch from f3f77f9 to 2f3c2da Compare May 26, 2017 03:00

frasertweedale force-pushed the fix/long-oid-parse-failure branch from 2f3c2da to effeb60 Compare May 29, 2017 02:16

tiran suggested changes May 29, 2017

View reviewed changes

frasertweedale commented May 29, 2017

View reviewed changes

tiran mentioned this pull request May 29, 2017

bpo-30502: Fix handling of long oids python/cpython#1852

Closed

reaperhulk approved these changes May 29, 2017

View reviewed changes

reaperhulk merged commit d607dd7 into pyca:master May 29, 2017

frasertweedale deleted the fix/long-oid-parse-failure branch May 29, 2017 23:56

github-actions Bot locked as resolved and limited conversation to collaborators Aug 18, 2020

Conversation

frasertweedale commented May 26, 2017

Uh oh!

mention-bot commented May 26, 2017

Uh oh!

alex commented May 26, 2017

Uh oh!

reaperhulk commented May 26, 2017

Uh oh!

frasertweedale commented May 26, 2017

Uh oh!

alex commented May 26, 2017

Uh oh!

stevepeak commented May 26, 2017

Uh oh!

alex commented May 26, 2017

Uh oh!

frasertweedale commented May 29, 2017

Uh oh!

alex commented May 29, 2017

Uh oh!

frasertweedale commented May 29, 2017 via email

Uh oh!

alex commented May 29, 2017

Uh oh!

tiran May 29, 2017

Choose a reason for hiding this comment

Uh oh!

reaperhulk May 29, 2017

Choose a reason for hiding this comment

Uh oh!

frasertweedale May 29, 2017

Choose a reason for hiding this comment

Uh oh!

reaperhulk May 29, 2017

Choose a reason for hiding this comment

Uh oh!

tiran commented May 29, 2017

test case

output

Uh oh!

frasertweedale commented May 29, 2017

Uh oh!

reaperhulk left a comment

Choose a reason for hiding this comment

Uh oh!

reaperhulk commented May 29, 2017

Uh oh!

alex commented May 29, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants