5 ms·

It's a shame this is being upvoted so highly when it's factually incorrect. A rebuttal can be found here: http://nmav.gnutls.org/2011/05/is-really-gnutls-consid

by euank 13y ago

It's a shame this is being upvoted so highly when it's factually incorrect. A rebuttal can be found here: http://nmav.gnutls.org/2011/05/is-really-gnutls-considered-harmful.html http://nmav.gnutls.org/2011/05/is-really-gnutls-considered-h... It's rather silly that the news of a critical bug in GnuTLS that was caused by a goto somehow makes non-news and factually wrong information from 5 years ago popular.

Xylakant 13y ago
If you dive into the exchange following http://www.openldap.org/lists/openldap-devel/200802/msg00100.html http://www.openldap.org/lists/openldap-devel/200802/msg00100... you'll see that the criticism does not only allude to the liberal use of strcat() and strcopy() but has more fundamental problems about the general quality and efficiency of the code. While I do agree that digging up a 5 year old rant with a catchy tagline does not tell us everything about the projects current state, the rebuttal also misses some of the points brought up in the initial criticism
mikeash 13y ago
That rebuttal doesn't address the more important claim that the library uses NUL-terminated strings for potentially arbitrary binary data. Not that this affects your point, but the critical bug was not caused by a goto. Rather, it was caused by a mismatch in return value semantics, where a variable was used to store a value where 0 meant success, and then later used to return a value where non-zero meant success.
simias 13y ago
It addresses the use of the libc str* functions but not the first point about the prototype of set_subject_alt_name. It's also discussed later in the thread: http://www.openldap.org/lists/openldap-devel/200802/msg00100.html http://www.openldap.org/lists/openldap-devel/200802/msg00100... > You note that there's really a small number of instances of strcat() in the code. That's true, but that's because you've provided your own _gnutls_str_cat() function instead, which is also heavily used. Assuming that strlen() isn't going to SEGV on you (which depends on dumb luck) this becomes just a question of efficiency. I also think that in the rebuttal the example is extremely poorly chosen since the code is equivalent to the much simpler char str[256] = "PKIX1.CRLDistributionPoints.?1.distributionPoint.fullName"; Assuming you even need str to be 256 char long, otherwise you would use 'char str[] = "...";' or 'const char *str = ' if you don't modify the string. Maybe the use of the concatenation is legitimate in the real code but I cannot judge that since it appears to have changed since the article was written: https://gitorious.org/gnutls/gnutls/source/d9ce82a4ce690857f828e6c434718c2a85bfbab0:lib/x509/extensions.c#L1041 https://gitorious.org/gnutls/gnutls/source/d9ce82a4ce690857f... No strcat in there. Maybe it wasn't such a good idea after all? :) EDIT: Actually, I dug into the git repo to find the old code and looked to revert to a commit around the date the blog post was written. Obviously I don't intend to get any work done this afternoon. I found this commit on the same day (2011/05/10): https://gitorious.org/gnutls/gnutls/commit/3df051196838f4f43f2350c637bb3fae3001ddaf https://gitorious.org/gnutls/gnutls/commit/3df051196838f4f43... "eliminated last instances of strcpy() and strcat() to keep pendantics happy." So the reason he says the problem is not here anymore is because he fixed it just before writing this blog post, more than 3 years after the openldap rant. I'm sure nmav was well-intentioned but it does weaken his rebuttal somewhat.
datenwolf 13y ago
> A rebuttal can be found here Only that this rebuttal completely misses the point. They misunderstood the criticism being about buffer overflow vulnerability >> So what is the issue? Howard claims that GnuTLS makes liberal use of strcpy(), strcat() and strlen(). Those functions are known to be responsible for several attacks via buffer overflows in current programs. while it was in fact about the nature of the data to be processed, namely that it may not be NUL terminated strings but arbitrary binary data for which the whole bunch of `str…` functions and any other string processing that expects to operate on NUL terminated strings will miserably fail > Looking across more of their APIs, I see that the code makes liberal use of strlen and strcat, when it needs to be using counted-length data blobs everywhere. In short, the code is fundamentally broken; most of its external and internal APIs are incapable of passing binary data without mangling it. The code is completely unsafe for handling binary data, and yet the nature of TLS processing is almost entirely dependent on secure handling of binary data.
mcguire 13y ago
I was a bit curious about this quote: "It turns out that their corresponding set_subject_alt_name() API only takes a char \ pointer as input, without a corresponding length. As such, this API will only work for string-form alternative names, and will typically break with IP addresses and other alternatives." Yes, an API designed for strings will break if you pass it a struct in_addr or something, but it should be fine with a dotted-decimal string, right?
- simias 13y ago
  Maybe he means a binary representation of an IP address (4 bytes for an IPv4). In this case serializing 10.0.0.1 would break.
  - ambrice 13y ago
    Yes, lots of APIs that take char * will break if you pass them some arbitrary other type of data that you assume they will handle.
    - simonh 13y ago
      How about if they are implementing a public standard that states this other arbitrary type of data is valid?
      - ambrice 13y ago
        It is a logical leap to assume that because the spec says other types of data are valid that means you should be able to pass arbitrary data to this function that is documented as requiring a 0 terminated string. Let's say you wanted to pass an IPv4 address. Would you expect to pass it a uint32_t pointer? A struct in_addr pointer? Host or network byte order?
        deleted 13y ago
        [deleted]
- 13y ago
dredmorbius 13y ago
Upvotes have many reasons. Sometimes it's to find out what the discussion will reveal (my own motivation here).
el_duderino 13y ago
Exactly. I came to say the same thing.
mishikal 13y ago
There was an extensive discussion between Howard and nmav on that blog post a few months ago. nmav has completely deleted that discussion from the blog because he didn't like the fact that additional problems with the GnuTLS code based were pointed out in that discussion. It is quite interesting that someone who is so tied into an open source project is against keeping a public discussion available to the world.
- hyc_symas 13y ago
  Misremembered, the discussion was on G+ https://plus.google.com/112912252727709520367/posts/RGBXrLTh7oG https://plus.google.com/112912252727709520367/posts/RGBXrLTh...
hyc_symas 13y ago
The rebuttal completely missed the point. https://plus.google.com/112912252727709520367/posts/RGBXrLTh7oG https://plus.google.com/112912252727709520367/posts/RGBXrLTh...

by euank 13y ago

Xylakant 13y ago

If you dive into the exchange following http://www.openldap.org/lists/openldap-devel/200802/msg00100.html http://www.openldap.org/lists/openldap-devel/200802/msg00100... you'll see that the criticism does not only allude to the liberal use of strcat() and strcopy() but has more fundamental problems about the general quality and efficiency of the code. While I do agree that digging up a 5 year old rant with a catchy tagline does not tell us everything about the projects current state, the rebuttal also misses some of the points brought up in the initial criticism

mikeash 13y ago

That rebuttal doesn't address the more important claim that the library uses NUL-terminated strings for potentially arbitrary binary data. Not that this affects your point, but the critical bug was not caused by a goto. Rather, it was caused by a mismatch in return value semantics, where a variable was used to store a value where 0 meant success, and then later used to return a value where non-zero meant success.

simias 13y ago

It addresses the use of the libc str* functions but not the first point about the prototype of set_subject_alt_name. It's also discussed later in the thread: http://www.openldap.org/lists/openldap-devel/200802/msg00100.html http://www.openldap.org/lists/openldap-devel/200802/msg00100... > You note that there's really a small number of instances of strcat() in the code. That's true, but that's because you've provided your own _gnutls_str_cat() function instead, which is also heavily used. Assuming that strlen() isn't going to SEGV on you (which depends on dumb luck) this becomes just a question of efficiency. I also think that in the rebuttal the example is extremely poorly chosen since the code is equivalent to the much simpler char str[256] = "PKIX1.CRLDistributionPoints.?1.distributionPoint.fullName"; Assuming you even need str to be 256 char long, otherwise you would use 'char str[] = "...";' or 'const char *str = ' if you don't modify the string. Maybe the use of the concatenation is legitimate in the real code but I cannot judge that since it appears to have changed since the article was written: https://gitorious.org/gnutls/gnutls/source/d9ce82a4ce690857f828e6c434718c2a85bfbab0:lib/x509/extensions.c#L1041 https://gitorious.org/gnutls/gnutls/source/d9ce82a4ce690857f... No strcat in there. Maybe it wasn't such a good idea after all? :) EDIT: Actually, I dug into the git repo to find the old code and looked to revert to a commit around the date the blog post was written. Obviously I don't intend to get any work done this afternoon. I found this commit on the same day (2011/05/10): https://gitorious.org/gnutls/gnutls/commit/3df051196838f4f43f2350c637bb3fae3001ddaf https://gitorious.org/gnutls/gnutls/commit/3df051196838f4f43... "eliminated last instances of strcpy() and strcat() to keep pendantics happy." So the reason he says the problem is not here anymore is because he fixed it just before writing this blog post, more than 3 years after the openldap rant. I'm sure nmav was well-intentioned but it does weaken his rebuttal somewhat.

datenwolf 13y ago

> A rebuttal can be found here Only that this rebuttal completely misses the point. They misunderstood the criticism being about buffer overflow vulnerability >> So what is the issue? Howard claims that GnuTLS makes liberal use of strcpy(), strcat() and strlen(). Those functions are known to be responsible for several attacks via buffer overflows in current programs. while it was in fact about the nature of the data to be processed, namely that it may not be NUL terminated strings but arbitrary binary data for which the whole bunch of `str…` functions and any other string processing that expects to operate on NUL terminated strings will miserably fail > Looking across more of their APIs, I see that the code makes liberal use of strlen and strcat, when it needs to be using counted-length data blobs everywhere. In short, the code is fundamentally broken; most of its external and internal APIs are incapable of passing binary data without mangling it. The code is completely unsafe for handling binary data, and yet the nature of TLS processing is almost entirely dependent on secure handling of binary data.

mcguire 13y ago

I was a bit curious about this quote: "It turns out that their corresponding set_subject_alt_name() API only takes a char \ pointer as input, without a corresponding length. As such, this API will only work for string-form alternative names, and will typically break with IP addresses and other alternatives." Yes, an API designed for strings will break if you pass it a struct in_addr or something, but it should be fine with a dotted-decimal string, right?

simias 13y ago
Maybe he means a binary representation of an IP address (4 bytes for an IPv4). In this case serializing 10.0.0.1 would break.
- ambrice 13y ago
  Yes, lots of APIs that take char * will break if you pass them some arbitrary other type of data that you assume they will handle.
  - simonh 13y ago
    How about if they are implementing a public standard that states this other arbitrary type of data is valid?
    - ambrice 13y ago
      It is a logical leap to assume that because the spec says other types of data are valid that means you should be able to pass arbitrary data to this function that is documented as requiring a 0 terminated string. Let's say you wanted to pass an IPv4 address. Would you expect to pass it a uint32_t pointer? A struct in_addr pointer? Host or network byte order?
      - deleted 13y ago
        [deleted]
13y ago

dredmorbius 13y ago

Upvotes have many reasons. Sometimes it's to find out what the discussion will reveal (my own motivation here).

el_duderino 13y ago

Exactly. I came to say the same thing.

mishikal 13y ago

There was an extensive discussion between Howard and nmav on that blog post a few months ago. nmav has completely deleted that discussion from the blog because he didn't like the fact that additional problems with the GnuTLS code based were pointed out in that discussion. It is quite interesting that someone who is so tied into an open source project is against keeping a public discussion available to the world.

hyc_symas 13y ago
Misremembered, the discussion was on G+ https://plus.google.com/112912252727709520367/posts/RGBXrLTh7oG https://plus.google.com/112912252727709520367/posts/RGBXrLTh...

hyc_symas 13y ago

The rebuttal completely missed the point. https://plus.google.com/112912252727709520367/posts/RGBXrLTh7oG https://plus.google.com/112912252727709520367/posts/RGBXrLTh...