CString::Base64Decode performs (c << N) on a signed char whose value is -1 whenever the input contains a byte that's outside the base64 alphabet. The decode lookup table stores XX = 0xff as the sentinel for invalid bytes; that sentinel is then read through (char) (signed on every platform ZNC supports), making the shifts UB under C++17 (still UB pre-C++20).
Three nearby sites have the same pattern:
// src/ZNCString.cpp
1083:unsignedlongCString::Base64Decode(CString&sRet)const{...1095:for(i=0,p=out;i<uLen;i++){1096:c=(char)base64_table[(unsignedchar)in[i++]];1097:c1=(char)base64_table[(unsignedchar)in[i++]];1098:*p++=char((c<<2)|((c1>>4)&0x3));// <-- UB when c == -1
...1104:c=(char)base64_table[(unsignedchar)in[i]];1105:*p++=char(((c1<<4)&0xf0)|((c>>2)&0xf));// <-- UB when c1 == -1
...1112:*p++=char(((c<<6)&0xc0)|// <-- UB when c == -1
1113:(char)base64_table[(unsignedchar)in[i]]);
Reachability
Base64Decode is called pre-auth from CHTTPSock::ReadLine for the value of
an Authorization: header (src/HTTPSock.cpp:140):
Any unauthenticated client can reach it. A Basic auth value containing ~, ,, *, etc. (anything mapped to the XX sentinel) triggers the UB.
In production builds without UBSan or -ftrapv, arm64/x86 just execute the shift on the wrapped pattern and produce a garbage decoded byte, after which authentication fails with no externally visible difference. So the practical production impact is nil. The bug shows up as a hard abort in any build that runs UBSan (CIFuzz, OSS-Fuzz, distros that enable -ftrapv, or anyone running ZNC under a sanitizer for debugging).
Reproduction
Build a small standalone driver against the existing libznc.a:
ZNCString.cpp:1098:24: runtime error: left shift of negative value -1
input ~AAA -> decoded 3 bytes
ZNCString.cpp:1105:30: runtime error: left shift of negative value -1
input A~AA -> decoded 3 bytes
ZNCString.cpp:1112:29: runtime error: left shift of negative value -1
input AA~A -> decoded 3 bytes
Same UB also reachable via the fuzzer driver against a complete HTTP
request, no auth needed:
printf'GET / HTTP/1.0\r\nAuthorization: Basic AA~A\r\n\r\n' > req
./fuzz_http req
# ZNCString.cpp:1112:29: runtime error: left shift of negative value -1# ... CString::Base64Decode# ... CHTTPSock::ReadLine HTTPSock.cpp:140
Fix
Cast to unsigned char before shifting so the operand is non-negative:
c=base64_table[(unsignedchar)in[i++]];// c is unsigned char, value 0..0xff
c1=base64_table[(unsignedchar)in[i++]];*p++=char(((unsigned)c<<2)|((c1>>4)&0x3));...*p++=char(((unsigned)c1<<4)|((c>>2)&0xf));...*p++=char(((unsigned)c<<6)|base64_table[(unsignedchar)in[i]]);
Or alternatively, treat the XX sentinel as an explicit decode failure
(currently the function silently produces garbage on invalid input).
Happy to send a PR if useful.
`CString::Base64Decode` performs `(c << N)` on a signed `char` whose value is `-1` whenever the input contains a byte that's outside the base64 alphabet. The decode lookup table stores `XX = 0xff` as the sentinel for invalid bytes; that sentinel is then read through `(char)` (signed on every platform ZNC supports), making the shifts UB under C++17 (still UB pre-C++20).
Three nearby sites have the same pattern:
```cpp
// src/ZNCString.cpp
1083: unsigned long CString::Base64Decode(CString& sRet) const {
...
1095: for (i = 0, p = out; i < uLen; i++) {
1096: c = (char)base64_table[(unsigned char)in[i++]];
1097: c1 = (char)base64_table[(unsigned char)in[i++]];
1098: *p++ = char((c << 2) | ((c1 >> 4) & 0x3)); // <-- UB when c == -1
...
1104: c = (char)base64_table[(unsigned char)in[i]];
1105: *p++ = char(((c1 << 4) & 0xf0) | ((c >> 2) & 0xf));// <-- UB when c1 == -1
...
1112: *p++ = char(((c << 6) & 0xc0) | // <-- UB when c == -1
1113: (char)base64_table[(unsigned char)in[i]]);
```
## Reachability
`Base64Decode` is called pre-auth from `CHTTPSock::ReadLine` for the value of
an `Authorization:` header (`src/HTTPSock.cpp:140`):
```cpp
138: } else if (sName.Equals("Authorization:")) {
139: CString sUnhashed;
140: sLine.Token(2).Base64Decode(sUnhashed);
```
Any unauthenticated client can reach it. A `Basic` auth value containing `~`, `,`, `*`, etc. (anything mapped to the `XX` sentinel) triggers the UB.
In production builds without UBSan or `-ftrapv`, arm64/x86 just execute the shift on the wrapped pattern and produce a garbage decoded byte, after which authentication fails with no externally visible difference. So the practical production impact is nil. The bug shows up as a hard abort in any build that runs UBSan (CIFuzz, OSS-Fuzz, distros that enable `-ftrapv`, or anyone running ZNC under a sanitizer for debugging).
## Reproduction
Build a small standalone driver against the existing `libznc.a`:
```bash
clang++ -std=c++17 -fsanitize=address,undefined -fno-omit-frame-pointer -g -O1 \
-I include -I build/include -include znc/zncconfig.h \
poc_b64.cpp build/src/libznc.a \
-lpthread -ldl -framework CoreFoundation -L/opt/homebrew/lib -largon2 \
-o poc_b64
```
`poc_b64.cpp`:
```cpp
#include <cstdio>
#include <znc/ZNCString.h>
int main() {
for (auto in : {"~AAA", "A~AA", "AA~A"}) {
CString s(in), out;
s.Base64Decode(out);
fprintf(stderr, "input %s -> decoded %zu bytes\n", in, out.size());
}
}
```
Output (UBSAN_OPTIONS=halt_on_error=0):
```
ZNCString.cpp:1098:24: runtime error: left shift of negative value -1
input ~AAA -> decoded 3 bytes
ZNCString.cpp:1105:30: runtime error: left shift of negative value -1
input A~AA -> decoded 3 bytes
ZNCString.cpp:1112:29: runtime error: left shift of negative value -1
input AA~A -> decoded 3 bytes
```
Same UB also reachable via the fuzzer driver against a complete HTTP
request, no auth needed:
```bash
printf 'GET / HTTP/1.0\r\nAuthorization: Basic AA~A\r\n\r\n' > req
./fuzz_http req
# ZNCString.cpp:1112:29: runtime error: left shift of negative value -1
# ... CString::Base64Decode
# ... CHTTPSock::ReadLine HTTPSock.cpp:140
```
## Fix
Cast to `unsigned char` before shifting so the operand is non-negative:
```cpp
c = base64_table[(unsigned char)in[i++]]; // c is unsigned char, value 0..0xff
c1 = base64_table[(unsigned char)in[i++]];
*p++ = char(((unsigned)c << 2) | ((c1 >> 4) & 0x3));
...
*p++ = char(((unsigned)c1 << 4) | ((c >> 2) & 0xf));
...
*p++ = char(((unsigned)c << 6) | base64_table[(unsigned char)in[i]]);
```
Or alternatively, treat the `XX` sentinel as an explicit decode failure
(currently the function silently produces garbage on invalid input).
Happy to send a PR if useful.
<!-- github-sync-id:4326795756 -->
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
CString::Base64Decodeperforms(c << N)on a signedcharwhose value is-1whenever the input contains a byte that's outside the base64 alphabet. The decode lookup table storesXX = 0xffas the sentinel for invalid bytes; that sentinel is then read through(char)(signed on every platform ZNC supports), making the shifts UB under C++17 (still UB pre-C++20).Three nearby sites have the same pattern:
Reachability
Base64Decodeis called pre-auth fromCHTTPSock::ReadLinefor the value ofan
Authorization:header (src/HTTPSock.cpp:140):Any unauthenticated client can reach it. A
Basicauth value containing~,,,*, etc. (anything mapped to theXXsentinel) triggers the UB.In production builds without UBSan or
-ftrapv, arm64/x86 just execute the shift on the wrapped pattern and produce a garbage decoded byte, after which authentication fails with no externally visible difference. So the practical production impact is nil. The bug shows up as a hard abort in any build that runs UBSan (CIFuzz, OSS-Fuzz, distros that enable-ftrapv, or anyone running ZNC under a sanitizer for debugging).Reproduction
Build a small standalone driver against the existing
libznc.a:poc_b64.cpp:Output (UBSAN_OPTIONS=halt_on_error=0):
Same UB also reachable via the fuzzer driver against a complete HTTP
request, no auth needed:
Fix
Cast to
unsigned charbefore shifting so the operand is non-negative:Or alternatively, treat the
XXsentinel as an explicit decode failure(currently the function silently produces garbage on invalid input).
Happy to send a PR if useful.
Is this different from #2001 ?
Hi @DarthGandalf, they are the same since I mistakenly opened the issue twice. :( So I closed #2001.