Hacker News Reader

Comment by euank | Hacker News Reader

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-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.

View on HN

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...

> 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/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/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.

> 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.

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.

If you dive into the exchange following 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

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.

Misremembered, the discussion was on G+ https://plus.google.com/112912252727709520367/posts/RGBXrLTh...

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?

The issue is that they designed a API taking a NUL terminated string in the first place, as it should have been something more generic. They knew little enough of X.509 they didn't bother to handle every cases.

My understanding of RFC 3280 is pretty old, but the relevant ASN.1 type describing a subjectAltName seems to be :

SubjectAltName ::= GeneralNames

GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName

GeneralName ::= CHOICE { otherName [0] AnotherName, rfc822Name [1] IA5String, dNSName [2] IA5String, x400Address [3] ORAddress, directoryName [4] Name, ediPartyName [5] EDIPartyName, uniformResourceIdentifier [6] IA5String, iPAddress [7] OCTET STRING, registeredID [8] OBJECT IDENTIFIER }

The IP address case is represented as an octet string, and the octet 0 is legitimate, making their API broken...

That's the X.509 certificate format, right? It's not a code interface.

My point was that it's not reasonable to expect an interface that appears to be accepting a string to also accept random bytes; "10.0.0.8" isn't the same as 0x0a000008.

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.

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.

How about if they are implementing a public standard that states this other arbitrary type of data is valid?

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?

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

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

Exactly. I came to say the same thing.

Reply on news.ycombinator.com