Fix buffer overflows in SNMP MIB parser token handling by neosys007 · Pull Request #2391 · squid-cache/squid

neosys007 · 2026-03-16T07:46:32Z

The get_token() function in lib/snmplib/parse.c does not
receive a buffer size parameter and writes to the caller's
buffer without bounds checking. Callers use fixed 64-byte stack
buffers (token[], syntax[], nexttoken[]), which can be
overflowed by oversized tokens from malicious MIB files.

This vulnerability could be triggered by a malicious MIB file
containing an oversized identifier in a SEQUENCE OF declaration,
leading to stack buffer overflow.

The get_token() function in lib/snmplib/parse.c does not receive a buffer size parameter and writes to the caller's buffer without bounds checking. Callers use fixed 64-byte stack buffers (token[], syntax[], nexttoken[]), which can be overflowed by oversized tokens from malicious MIB files. This fix: 1. Adds MAX_TOKEN_SIZE constant (64 bytes) matching caller buffer sizes 2. Adds bounds checking in get_token() before writing to the token buffer 3. Replaces unsafe strcpy/strcat with snprintf in parse_objecttype() The vulnerability could be triggered by a malicious MIB file containing an oversized identifier in a SEQUENCE OF declaration, leading to stack buffer overflow. Signed-off-by: hou pengpeng <pengpeng@iscas.ac.cn>

yadij · 2026-03-16T09:50:53Z

Fixed line wrapping and removed list duplicating what the patch says.

yadij · 2026-03-16T10:04:26Z

lib/snmplib/parse.c

 #define QUOTE       37

+/* Maximum token buffer size - must match caller's buffer size */
+#define MAX_TOKEN_SIZE 64


Don't just document as used for buffers. This constant needs to actually be used to declare the buffers whose size it controls.

If the snmp_mib_tree::label buffer is one of those, then this constant will need to be placed in include/parse.h.

Also, due to how this is being used the documentation needs to say "including NUL terminator" to help avoid future mistakes.

Agreed. I will remove MAX_TOKEN_SIZE in v2 rather than trying to reuse it for multiple buffers. The next revision will make get_token() capacity-aware and have callers pass their actual buffer size explicitly.

rousskov

Thank you for working on this bug fix.

rousskov · 2026-03-16T13:36:15Z

lib/snmplib/parse.c

 #define QUOTE       37

+/* Maximum token buffer size - must match caller's buffer size */
+#define MAX_TOKEN_SIZE 64


Please do not add this constant. It does not prevent future problems like the ones fixed by this PR, it does not expose the dangers in caller contexts, and it does not match the actual buffer sizes in existing code (e.g., getoid() tokens are 128 bytes long, not 64 bytes AFAICT).

Instead, add bufSize or bufCapacity parameter to get_token() and adjust get_token() callers to pass sizeof(token) or similar values, as appropriate. We could do better in C++, but I do not know of a better solution for this C code.

Agreed.

I will remove the constant in the next revision and change get_token() to take a caller-provided buffer capacity instead.

I will also audit and update all get_token() callers to pass their actual destination size (including the larger getoid() token buffers), rather than assuming a shared fixed size.

rousskov · 2026-03-16T13:52:56Z

lib/snmplib/parse.c

+                if (cp - token < MAX_TOKEN_SIZE - 1)
+                    *cp++ = ch;


Please do not ignore problems -- this if statement needs an else clause that would immediately return a parsing error instead.

I have not checked carefully, but I suspect the best way to handle this would be to add an ERROR token type with -2 as a value. One could abuse existing CONTINUE for this, but that would be too misleading IMO. tokens[] array may need to be adjusted. print_error() will need to be adjusted. Other adjustments may be needed, but all the callers I have checked should handle the new type correctly without changes; I have not checked all the callers though; please do that.

Agreed.

I will not keep the current "skip/truncate" behavior in the next revision.

I agree that reusing CONTINUE here would be misleading, so I will use a dedicated error token instead. I will rework this to return an explicit parsing error on token overflow, update the related handling around tokens[] and print_error(), and audit all get_token() callers to make sure the new error path is handled consistently.

rousskov · 2026-03-16T13:53:08Z

lib/snmplib/parse.c

+            if (cp - token < MAX_TOKEN_SIZE - 1)
+                *cp++ = ch;


Please do not ignore problems -- this if statement needs an else clause that would immediately return a parsing error instead. See another change request for details.

Agreed.

I will remove this silent truncation logic in the next revision. On overflow, get_token() will return an explicit parsing error immediately instead of continuing.

rousskov · 2026-03-16T13:58:11Z

lib/snmplib/parse.c

    switch (type) {
    case SEQUENCE:
-        strcpy(syntax, token);
+        snprintf(syntax, sizeof(syntax), "%s", token);


If you are adding snprintf() calls, please do not ignore call failures. It may not be trivial, but it is necessary. This concern applies to all snprintf() calls added in this PR.

Agreed. I will remove these added snprintf() calls in v2 and fix this at the get_token() boundary instead.

rousskov · 2026-03-16T14:01:24Z

lib/snmplib/parse.c

+            if (len < sizeof(syntax) - 1) {
+                snprintf(syntax + len, sizeof(syntax) - len, " %s", nexttoken);


Please do not duplicate snprintf() protections manually. Check for snprintf() failures instead of adding another if without an error-handling else clause.

You may explicitly assert that len <= sizeof(syntax) to avoid doubts about unsigned underflows in snpritf() call argument computation. Preceding code ought to make that assertion true.

Agreed.

I will not keep this manual pre-check pattern in the next revision. I am reworking the fix so that these added snprintf()-based syntax-buffer changes go away, and any remaining snprintf() usage will check return values directly rather than relying on a separate size guard.

rousskov · 2026-03-16T14:17:35Z

I added "SNMP" to the PR title to provide more context and replaced "overflow" with "overflows" because this PR fixes several different overflow bugs AFAICT.

neosys007 · 2026-03-16T14:55:49Z

Thank you for the detailed review.

I agree with the requested direction.

I will rework this patch to:

remove MAX_TOKEN_SIZE,
add a buffer-capacity parameter to get_token(),
update all callers to pass their actual destination size,
return a parse error on oversized tokens instead of truncating/ignoring them,
check snprintf() return values directly, and
audit all get_token() callers, including the larger getoid() buffers.

The underlying bug report stays the same, but I agree the fix should be done at the get_token() API level rather than with a shared constant.

I will post an updated revision.

neosys007 · 2026-03-16T15:27:16Z

Posted v2 in commit 2c3603c.

This revision:

removes the shared MAX_TOKEN_SIZE constant,
makes get_token() take caller-provided buffer capacity,
updates callers to pass their actual destination size,
returns an explicit parse error on oversized tokens instead of truncating, and
removes the snprintf()-based workaround in favor of fixing this at the get_token() boundary.

I also audited the get_token() callers, including the larger getoid() token buffer.

neosys007 · 2026-03-19T02:15:44Z

I updated the PR to fix a real build failure in the current revision (address of register variable requested in get_token() after taking &cp).

The updated head is now 84bf7804f, and I also verified the lib/snmplib object builds locally.

The new workflow run appears to be waiting for maintainer approval. Could a maintainer please approve/re-run the PR checks?

rousskov · 2026-03-19T03:29:50Z

@neosys007, just FYI: As you fix CI tests, there is usually no need to squash and/or force-push your changes (and there is some minor harm/inconvenience in those actions). After your code gets approved, Anubis, our merge bot, will squash your commits before merging them into master.

If you do not want to wait for us to approve CI builds, you may run these tests in your repository. There are several ways to do that. One of them is to create a pull request in your repository -- it will be just for you to run CI tests, nobody from the Project will pay attention to it...

P.S. I will review and adjust your changes ASAP, but there is no need to wait for that to happen if you want to keep fixing CI failures.

neosys007 · 2026-03-19T11:37:36Z

Thank you, that is very helpful.

Understood. I will avoid further squash/force-push updates and use follow-up commits from here. I am investigating the remaining source-maintenance failure now, and may use a PR in my fork to run CI while narrowing it down.

Best regards,
Pengpeng Hou

neosys007 · 2026-03-19T11:45:24Z

I added a follow-up commit (17babaffa) to sync CONTRIBUTORS with the current source-maintenance output.

The new workflow run appears to be waiting for maintainer approval again. Could a maintainer please approve/re-run the PR checks?

rousskov · 2026-03-19T15:49:41Z

CI tests are all green. The ball is fully on my side now.

rousskov

PR description: This vulnerability could be triggered by a malicious MIB file containing an oversized identifier in a SEQUENCE OF declaration, leading to stack buffer overflow.

I believe the above assertion is false (and this PR changes are untested): AFAICT, no MIB file, no matter how malicious, can trigger this vulnerability because the code in question is unreachable. It is unreachable because it is unused since 1998 commit dba79ac. Squid hard-codes its MIB and, hence, never calls init_mib() which reads MIB and is the only function that (indirectly) calls get_token().

If you agree that get_token() is unreachable and unused, then please close this PR.

Needless to say, we do need to remove unused code from Squid. There is a lot of it!¹ We already have an active project that focuses on unused code removal. The corresponding code changes are currently stuck in the backlog, but they do remove get_token() among many other unused functions. PRs that focus on removing some of that code are not welcome at this point because they would create more conflicts for that backlogged project to deal with.

Which partially explains why it took me a while to notice that this PR modifies an unused function. I am sorry for not realizing this sooner. This delay is one of the many reasons we need to test pull requests before posting them! ↩

squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Mar 16, 2026

This comment was marked as resolved.

Sign in to view

squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Mar 16, 2026

yadij requested changes Mar 16, 2026

View reviewed changes

yadij added the S-waiting-for-author author action is expected (and usually required) label Mar 16, 2026

rousskov requested changes Mar 16, 2026

View reviewed changes

rousskov changed the title ~~Fix buffer overflow in MIB parser token handling~~ Fix buffer overflows in SNMP MIB parser token handling Mar 16, 2026

rousskov self-requested a review March 16, 2026 15:57

rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Mar 16, 2026

SNMP: make get_token() buffer-capacity-aware

84bf780

neosys007 force-pushed the fix-mib-parser-buffer-overflow branch from 2c3603c to 84bf780 Compare March 19, 2026 00:57

Maintenance: update CONTRIBUTORS for PR authors

17babaf

rousskov requested changes Mar 19, 2026

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Mar 19, 2026

		if (len < sizeof(syntax) - 1) {
		snprintf(syntax + len, sizeof(syntax) - len, " %s", nexttoken);

Conversation

neosys007 commented Mar 16, 2026 • edited by yadij Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

yadij commented Mar 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov commented Mar 16, 2026

Uh oh!

neosys007 commented Mar 16, 2026

Uh oh!

neosys007 commented Mar 16, 2026

Uh oh!

neosys007 commented Mar 19, 2026

Uh oh!

rousskov commented Mar 19, 2026

Uh oh!

neosys007 commented Mar 19, 2026

Uh oh!

neosys007 commented Mar 19, 2026

Uh oh!

rousskov commented Mar 19, 2026

Uh oh!

rousskov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

neosys007 commented Mar 16, 2026 •

edited by yadij

Loading

rousskov left a comment •

edited

Loading