clang-tidy: enable bugprone-signed-char-misuse, fix fallouts#20654
clang-tidy: enable bugprone-signed-char-misuse, fix fallouts#20654vszakats wants to merge 2 commits intocurl:masterfrom
bugprone-signed-char-misuse, fix fallouts#20654Conversation
|
augment review |
🤖 Augment PR SummarySummary: Enables clang-tidy’s 🤖 Was this summary useful? React with 👍 or 👎 |
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
See details in the comment below. Analyzed PR: #20654 at commit |
| if(content_type == SSL3_RT_CHANGE_CIPHER_SPEC) { | ||
| msg_type = *(const char *)buf; | ||
| msg_type = *(const unsigned char *)buf; | ||
| msg_name = "Change cipher spec"; | ||
| } | ||
| else if(content_type == SSL3_RT_ALERT) { | ||
| msg_type = (((const char *)buf)[0] << 8) + ((const char *)buf)[1]; | ||
| msg_type = (((const unsigned char *)buf)[0] << 8) + | ||
| ((const unsigned char *)buf)[1]; | ||
| msg_name = SSL_alert_desc_string_long(msg_type); | ||
| } | ||
| else { | ||
| msg_type = *(const char *)buf; | ||
| msg_type = *(const unsigned char *)buf; | ||
| msg_name = ssl_msg_type(ssl_ver, msg_type); |
There was a problem hiding this comment.
1. 🔵 Out-of-bounds read in OpenSSL message trace callback (ossl_trace) when record length is too short
| Property | Value |
|---|---|
| Severity | Low |
| CWE | CWE-125 |
Description
In ossl_trace() (registered via SSL_CTX_set_msg_callback when verbose tracing is enabled), the code unconditionally reads the first byte of buf and, for alerts, the first two bytes:
msg_type = *(const unsigned char *)buf;msg_type = (((const unsigned char *)buf)[0] << 8) + ((const unsigned char *)buf)[1];
There is no guard that len is at least 1 (or 2 for alerts) before these dereferences.
If OpenSSL invokes the msg callback with a short/empty record payload (e.g., a zero-length TLS record, or a malformed short alert record), this will read past the end of the provided buffer.
Impact:
- Remote-triggerable crash (DoS) in applications that enable libcurl verbose tracing with the OpenSSL msg callback.
- Potential information disclosure of adjacent memory via the computed
msg_typeincluded in debug output (though only in verbose/debug logging mode).
Vulnerable code:
if(content_type == SSL3_RT_CHANGE_CIPHER_SPEC) {
msg_type = *(const unsigned char *)buf;
}
else if(content_type == SSL3_RT_ALERT) {
msg_type = (((const unsigned char *)buf)[0] << 8) +
((const unsigned char *)buf)[1];
}
else {
msg_type = *(const unsigned char *)buf;
}Recommendation
Add length guards before dereferencing buf, and handle short records safely (e.g., log "short record" / "truncated alert" without reading bytes).
Example fix:
if(len < 1)
return; /* or skip msg_type parsing */
if(content_type == SSL3_RT_ALERT) {
if(len < 2) {
msg_type = 0;
msg_name = "Truncated alert";
}
else {
const unsigned char *p = buf;
msg_type = (p[0] << 8) | p[1];
msg_name = SSL_alert_desc_string_long(msg_type);
}
}
else {
msg_type = *(const unsigned char *)buf;
...
}This ensures the trace callback never reads past the provided buffer regardless of what OpenSSL passes to it.
Reported-by: aisle-research-bot Bug: curl#20654 (comment)
Reported-by: aisle-research-bot Bug: curl#20654 (comment)
Reported-by: aisle-research-bot Bug: #20654 (comment) Closes #20656
bugprone-signed-char-misusebugprone-signed-char-misuse warnings
bugprone-signed-char-misuse warningsbugprone-signed-char-misuse, fix fallouts
```
lib/vtls/openssl.c:2585:18: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
2585 | msg_type = *(const char *)buf;
| ^
lib/vtls/openssl.c:2593:18: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
2593 | msg_type = *(const char *)buf;
| ^
tests/server/mqttd.c:514:10: warning: comparison between 'signed char' and 'unsigned char' [bugprone-signed-char-misuse]
514 | if(passwd_flag == (char)(conn_flags & passwd_flag)) {
| ^
tests/server/tftpd.c:362:13: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
362 | c = test->rptr[0];
| ^
tests/server/tftpd.c:454:9: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
454 | c = *p++; /* pick up a character */
| ^
src/tool_urlglob.c:272:46: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
272 | pat->c.ascii.letter = pat->c.ascii.min = min_c;
| ^
src/tool_urlglob.c:273:24: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
273 | pat->c.ascii.max = max_c;
| ^
tests/libtest/cli_h2_pausing.c:164:23: warning: suspicious usage of 'sizeof()' on an expression of pointer type [bugprone-sizeof-expression]
164 | memset(&resolve, 0, sizeof(resolve));
| ^
tests/libtest/cli_upload_pausing.c:158:23: warning: suspicious usage of 'sizeof()' on an expression of pointer type [bugprone-sizeof-expression]
158 | memset(&resolve, 0, sizeof(resolve));
| ^
tests/libtest/first.c:86:15: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
86 | coptopt = arg[optpos];
| ^
tests/tunit/../libtest/first.c:86:15: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
86 | coptopt = arg[optpos];
| ^
tests/unit/../libtest/first.c:86:15: warning: 'signed char' to 'int' conversion; consider casting to 'unsigned char' first. [bugprone-signed-char-misuse]
86 | coptopt = arg[optpos];
| ^
```
Ref: https://clang.llvm.org/extra/clang-tidy/checks/bugprone/signed-char-misuse.html
Reported-by: aisle-research-bot Bug: curl#20654 (comment) Closes curl#20656
Examples:
Also:
Ref: https://clang.llvm.org/extra/clang-tidy/checks/bugprone/signed-char-misuse.html