HI!
Did anyone already take not of this? Are parts of OpenLDAP's code affected?
[Bug c/27180] New: pointer arithmetic overflow handling broken http://gcc.gnu.org/ml/gcc-bugs/2006-04/msg01297.html
US-CERT - Vulnerability Note VU#162289: gcc silently discards some wraparound checks http://www.kb.cert.org/vuls/id/162289
Ciao, Michael.
Michael Ströder wrote:
HI!
Did anyone already take not of this? Are parts of OpenLDAP's code affected?
Looks like a really stupid way to do bounds checking. I've never seen it in OpenLDAP code, but I also haven't looked for it explicitly either. The examples would only ever work for a machine with 32 bit pointers, you'd get no meaningful safety check with 64 bit pointers. (In fact, it's unlikely to provide a meaningful check on most 32 bit platforms, since user memory tends to be mapped into the lower 31 bits of the address space. You would have to be overflowing by more than 2^31 for the check to catch anything. What idiot would write a check that's so fragile?)
[Bug c/27180] New: pointer arithmetic overflow handling broken http://gcc.gnu.org/ml/gcc-bugs/2006-04/msg01297.html
US-CERT - Vulnerability Note VU#162289: gcc silently discards some wraparound checks http://www.kb.cert.org/vuls/id/162289
Michael Ströder writes:
[Bug c/27180] New: pointer arithmetic overflow handling broken http://gcc.gnu.org/ml/gcc-bugs/2006-04/msg01297.html
That code, "(char *)buf + (unsigned long)-1", yields undefined behavior if buf points at an object smaller than (unsigned long)-1 bytes. Pointer arithmetic is only valid within a single object.
However the bug it is marked as a dup of, miscompiles valid code: int *start /* size 100 */, *tmp; ... for (tmp = start + 100; tmp > start; --tmp); OpenLDAP has code which scans a struct berval backwards from bv_val+bv_len to bv_val.
Hallvard B Furuseth wrote:
Michael Ströder writes:
[Bug c/27180] New: pointer arithmetic overflow handling broken http://gcc.gnu.org/ml/gcc-bugs/2006-04/msg01297.html
That code, "(char *)buf + (unsigned long)-1", yields undefined behavior if buf points at an object smaller than (unsigned long)-1 bytes. Pointer arithmetic is only valid within a single object.
However the bug it is marked as a dup of, miscompiles valid code: int *start /* size 100 */, *tmp; ... for (tmp = start + 100; tmp> start; --tmp); OpenLDAP has code which scans a struct berval backwards from bv_val+bv_len to bv_val.
Hm, that may explain some of the various gcc optimizer-related bugs we've encountered recently. According to the gcc bugzilla it was reported against 4.1 and 4.2, and fixed two years ago. So I expect any recent build of gcc should be OK here.
But the bug description is quite explicit, it was a problem with constant folding, e.g. "pointer + constant" compared to another "pointer + constant". OpenLDAP code scanning a struct berval would not be using any constants; bv_len is a variable. As such, I doubt that any struct berval processing loops are affected by this bug.
And for the record, checking for pointer wraparound is the wrong thing to check. All you want to know is if the new data will fit into the provided buffer. Therefore that is precisely what you should test. In plain terms, you can test this in two phases: 1) would this data ever fit in this buffer? 2) will the data fit in the buffer as it is now?
Given
char buf[MYSIZE]; ber_len_t len; /* length of current buffer content */ struct berval *in; /* passed in, to be moved into buf */
You just test: if ( in->bv_len > MYSIZE || in->bv_len + len > MYSIZE ) return FAIL;
If the new data is "too large" for whatever definition of "too large", you'll break out, without ever having to even think of word size issues. Writing code that depends on particular word sizes and arithmetic overflows and wraparounds is just asking for trouble.
Howard Chu hyc@symas.com writes:
And for the record, checking for pointer wraparound is the wrong thing to check. All you want to know is if the new data will fit into the provided buffer. Therefore that is precisely what you should test.
It's also the wrong thing to check according to the C standard, which says that creating a pointer that points outside its object (with a special exception for a pointer that points one element beyond the end of an array) invokes undefined behavior. The program is then permitted to turn your computer into orange goo or do something else unpleasant.
The root of this "bug" (I think it's highly arguable whether gcc breaking broken "security" checks can really be called a bug) is the same as the root of many other complaints about gcc's optimizer, namely that compilers are permitted to take advantage of the assumption that programs won't do something undefined to optimize code and gcc is always becoming more aggressive about optimizing code.
It's also important to note that gcc is only just catching up to commercial compilers here. In the discussion of this bug on the gcc list, a very long list of compilers were identified that do exactly the same optimization that gcc does, including AIX's xlc and Intel's compiler. Many have been doing this optimization for years. So any bugs in programs surfaced by this gcc behavior have been there for years and trigger with other compilers.
Howard Chu writes:
char buf[MYSIZE]; ber_len_t len; /* length of current buffer content */ struct berval *in; /* passed in, to be moved into buf */
You just test: if ( in->bv_len > MYSIZE || in->bv_len + len > MYSIZE ) return FAIL;
Except that in->bv_len + len can wrap around:-) In this case, use if ( in->bv_len > MYSIZE - len ) since len will be <= MYSIZE.
Hallvard B Furuseth wrote:
Howard Chu writes:
char buf[MYSIZE]; ber_len_t len; /* length of current buffer content */ struct berval *in; /* passed in, to be moved into buf */
You just test: if ( in->bv_len> MYSIZE || in->bv_len + len> MYSIZE ) return FAIL;
Except that in->bv_len + len can wrap around:-) In this case, use if ( in->bv_len> MYSIZE - len ) since len will be<= MYSIZE.
No. You missed the point. The first part of the if will catch an outsized in->bv_len. There is never wraparound on any real world buffer sizes. E.g. in a 32 bit platform you cannot have a 2GB data buffer because there's no address space left for the code or stack. Likewise for 64 bit.
Some conflicting messages going around here... This little point rather bigger than perhaps makes sense, but anyway:
Howard Chu wrote:
Hallvard B Furuseth wrote:
Howard Chu wrote:
char buf[MYSIZE]; ber_len_t len; /* length of current buffer content */ struct berval *in; /* passed in, to be moved into buf */
You just test: if ( in->bv_len> MYSIZE || in->bv_len + len> MYSIZE ) return FAIL;
Except that in->bv_len + len can wrap around:-) In this case, use if ( in->bv_len> MYSIZE - len ) since len will be<= MYSIZE.
No. You missed the point. The first part of the if will catch an outsized in->bv_len.
So does in->bv_len > MYSIZE - len, since you already have a bug in the program if MYSIZE < len ("length of current buffer content"). If you worry about that use if ( len > MYSIZE || in->bv_len > MYSIZE - len ) which actually expresses what you are looking for: 'len' is valid, range, and there is enough room left to append 'in'.
There is never wraparound on any real world buffer sizes. E.g. in a 32 bit platform you cannot have a 2GB data buffer because there's no address space left for the code or stack. Likewise for 64 bit.
Well, no space left for stack anyway. I've used a machine where code and data may have been separate address spaces.
However C allows size_t to be too small to reach the entire address space. That just means that C implementation won't support objects larger than ((size_t)-1) bytes. I wonder if Windows or DOS had a mode like that (32-bit address space, 16 bit size_t), and for all I know someone is doing it again today in a 32->64-bit transition mode.
And of course anyone can see that in->bv_len + len > MYSIZE is exactly equivalent to len > MYSIZE - in->bv_len
After checking in->bv_len <= MYSIZE and unless in->bv_len + len wraps around, yes.
Though if we run into size_t narrower than pointers, we might learn of that in a ruder way: slap_sl_free() could fail to detect that the pointer to be freed belongs to another context. If the C implementation also ensures that no object can cross a 2**32-byte boundary, 'ptr1 < ptr2' need only compare the least significant 32 bits of the pointers. (DOS did that, at least.)
On Tue, 8 Apr 2008, Hallvard B Furuseth wrote:
Howard Chu writes:
You just test: if ( in->bv_len > MYSIZE || in->bv_len + len > MYSIZE ) return FAIL;
Except that in->bv_len + len can wrap around:-) In this case, use if ( in->bv_len > MYSIZE - len ) since len will be <= MYSIZE.
No, you don't know whether len is <= MYSIZE, but you _do_ know that in->bv_len is less than MYSIZE from the first clause in the test. So: if ( in->bv_len > MYSIZE || len > MYSIZE - in->bv_len ) return FAIL;
Philip Guenther
Philip Guenther wrote:
On Tue, 8 Apr 2008, Hallvard B Furuseth wrote:
Howard Chu writes:
You just test: if ( in->bv_len> MYSIZE || in->bv_len + len> MYSIZE ) return FAIL;
Except that in->bv_len + len can wrap around:-) In this case, use if ( in->bv_len> MYSIZE - len ) since len will be<= MYSIZE.
No, you don't know whether len is<= MYSIZE, but you _do_ know that in->bv_len is less than MYSIZE from the first clause in the test. So: if ( in->bv_len> MYSIZE || len> MYSIZE - in->bv_len ) return FAIL;
Exactly.
And of course anyone can see that in->bv_len + len > MYSIZE is exactly equivalent to len > MYSIZE - in->bv_len
-
Hallvard B Furuseth -
Howard Chu -
Michael Ströder -
Philip Guenther -
Russ Allbery