Skip to content

cast to unsigned char in ctype calls on untrusted request bytes#3335

Open
sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:ctype-unsigned-char
Open

cast to unsigned char in ctype calls on untrusted request bytes#3335
sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:ctype-unsigned-char

Conversation

@sahvx655-wq

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number:

Problem Summary:

While tracing how a brpc redis server handles RESP command names, I saw the command name lowercased with tolower over a raw char. Those bytes come straight off the socket through RedisCommandParser::Consume, so any byte in 0x80-0xFF reaches the loop as a negative char. Passing a negative value other than EOF to a <ctype.h> function is undefined behaviour, because the argument has to be representable as unsigned char; on some libc builds the ctype macro indexes before the lookup table, which is exactly the kind of thing a fuzzer or a hardened build will flag. The same shape sits in both parser paths (the inline-command path and the bulk-string path) and in Str2HttpMethod, which hands the first byte of an attacker-supplied HTTP/2 :method pseudo-header to toupper.

The root cause is just the missing cast, and the codebase already knows the correct idiom: the array-size guard a few lines up uses std::isalpha(static_cast<unsigned char>(*pfc)), and uri.cpp casts before its ctype calls too. Left unfixed this is latent UB on every redis server and h2 endpoint, harmless on glibc today but not portable and noisy under UBSan, so I have brought the three sites in line with the existing idiom.

What is changed and the side effects?

Changed:

Cast each untrusted byte to unsigned char before the ctype call in src/brpc/redis_command.cpp (both command-name paths) and src/brpc/http_method.cpp. Valid ASCII commands and methods behave exactly as before, so there is no observable behaviour change to test against.

Side effects:

  • Performance effects: none.

  • Breaking backward compatibility: none.

Signed-off-by: sahvx655-wq <sahvx655@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant