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.
> 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
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:
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):
"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.
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.
"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
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.
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?
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.