Skip to content

Fix size_t overflow in Malloc() argument in ReadParams()#77

Merged
mcarbonneaux merged 1 commit intoFastCGI-Archives:masterfrom
ppisar:CVE-2025-23016-malloc
Nov 26, 2025
Merged

Fix size_t overflow in Malloc() argument in ReadParams()#77
mcarbonneaux merged 1 commit intoFastCGI-Archives:masterfrom
ppisar:CVE-2025-23016-malloc

Conversation

@ppisar
Copy link
Contributor

@ppisar ppisar commented May 19, 2025

There were still two issues after commit
b0eabca (Update fcgiapp.c, Fixing an integer overflow (CVE-2025-23016)):

  • Signed int overflow in "nameLen + valueLen + 2" expression.

  • Sizes of size_t and int types are in general unrelated.

This fix resolves both of the issues.

Related to CVE-2025-23016.
Resolve #67.

There were still two issues after commit
b0eabca (Update fcgiapp.c, Fixing an
integer overflow (CVE-2025-23016)):

* Signed int overflow in "nameLen + valueLen + 2" expression.

* Sizes of size_t and int types are in general unrelated.

This fix resolves both of the issues.

Related to CVE-2025-23016.
Resolve FastCGI-Archives#67.

Signed-off-by: Petr Písař <[email protected]>
@ppisar ppisar force-pushed the CVE-2025-23016-malloc branch from a6dd59b to 7c47639 Compare May 19, 2025 13:41
@mcarbonneaux mcarbonneaux merged commit 15443e1 into FastCGI-Archives:master Nov 26, 2025
4 checks passed
#include <memory.h> /* for memchr() */
#include <stdarg.h>
#include <stdio.h>
#include <stdint.h>
Copy link

@LeSpocky LeSpocky Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces an implicit dependency to at least C99.

Link: https://en.cppreference.com/w/c/header/stdint.html

Copy link

@LeSpocky LeSpocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with autotools, but shouldn't that new dependency somehow be reflected by the build system?

@mcarbonneaux
Copy link
Member

mcarbonneaux commented Nov 26, 2025

I'm not familiar with autotools, but shouldn't that new dependency somehow be reflected by the build system?

the build action seem to work.

but i have now the warning:

fcgiapp.c:1180:47: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Wsign-compare]
 1180 |             if (nameLen >= INT_MAX || nameLen >= SIZE_MAX) {
      |                                               ^~
fcgiapp.c:1196:49: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Wsign-compare]
 1196 |             if (valueLen >= INT_MAX || valueLen >= SIZE_MAX) {
      |                                                 ^~

@ppisar ppisar i think nameLen and valueLen must be size_t in place of int that are plateform depedente.

@ppisar
Copy link
Contributor Author

ppisar commented Nov 26, 2025

Of course the types used in this function are badly chosen. I'm glad you have find it out.

But don't forget that you won't escape the comparison: FCGX_GetChar() returns int, so you need to validate the range when assigning its return value to size_t and then when accumulating nameLen from the bytes.

By the way the signed comparison warning comes from the fact that your standard library defines the SIZE_MAX constant value with LU suffix.

Another option would be simply type-casting nameLen to uintmax_t in the comparison ((uintmax_t)nameLen >= SIZE_MAX) as the nameLen value cannot be negative.

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.

[Security vulnerability - memory corruption] Integer overflow into heap overflow

3 participants