Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix potential buffer overflow in session handling code. #471

Closed

Conversation

Quipyowert2
Copy link
Contributor

@Quipyowert2 Quipyowert2 commented Mar 30, 2021

This buffer overflow is fixed by including field width limits in the format strings passed to sscanf.

sscanf's %s and %[ format specifiers can overflow the output buffer unless a
field width limit is given. if sscanf encounters a string without
spaces (with %s) or without the characters in the set given (with %[) and this
string is longer than the output buffer, it will overflow the buffer.

  • What does this PR do?
    Fixes potential buffer overflow in session handling code.

Cppcheck 2.4.1 warnings:

fvwm\session.c:376:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s, "%*s %[^\n]", s1);
   ^
fvwm\session.c:1225:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s2, "%[^\n]", s1);
   ^
fvwm\session.c:1236:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s2, "%[^\n]", s1);
   ^
fvwm\session.c:1350:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s, "%*s %s", s1);
   ^
fvwm\session.c:1469:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s2, "%[^\n]", s1);
   ^
fvwm\session.c:1479:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s2, "%[^\n]", s1);
   ^
fvwm\session.c:1489:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s2, "%[^\n]", s1);
   ^
fvwm\session.c:1499:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s2, "%[^\n]", s1);
   ^
fvwm\session.c:1509:4: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
   sscanf(s2, "%[^\n]", s1);
   ^
fvwm\session.c:1523:5: warning: sscanf() without field width limits can crash with huge input data. [invalidscanf]
    sscanf (s+pos, "%s%n", s1, &pos1);
    ^

Cppcheck warning explanation:

CWE: 119
sscanf() without field width limits can crash with huge input data. Add a field width specifier to fix this problem.

Sample program that can crash:

#include <stdio.h>
int main()
{
    char c[5];
    scanf("%s", c);
    return 0;
}

Typing in 5 or more characters may make the program crash. The correct usage here is 'scanf("%4s", c);', as the maximum field width does not include the terminating null byte.
Source: http://linux.die.net/man/3/scanf
Source: http://www.opensource.apple.com/source/xnu/xnu-1456.1.26/libkern/stdio/scanf.c
Fixes #XXX

at the end of your commit message, where XXX should be replaced with the
relevant issue number.

If there is more than one issue fixed then use:

Fixes #XXX, fixes #YYY, fixes #ZZZ

This buffer overflow is fixed by including field width limits in the
format strings passed to sscanf.

sscanf's %s and %[ format specifiers can overflow the output buffer unless a
field width limit is given. if sscanf encounters a string without
spaces (with %s) or without the characters in the set given (with %[) and this
string is longer than the output buffer, it will overflow the buffer.
@ThomasAdam
Copy link
Member

Hi @Quipyowert2

Thanks -- but I'm not fixing this with magic numbers. If we were to do this properly, we wouldn't use sscanf() at all, and as I'm rewriting most of this soon anyway, I'm not keen to fix it. It's been this way for years and never been a problem -- usually because there's a transient nature to how this file is written/read in to begin with.

Can I check -- are you a user of fvwm itself?

@ThomasAdam ThomasAdam closed this Mar 30, 2021
@Quipyowert2
Copy link
Contributor Author

I used to use fvwm2 when I had a machine running Linux but on this laptop I mainly use Windows 10 now so I don't use fvwm much anymore as a primary window manager since Windows uses its own window manager (Desktop Window Manager). When I found out about fvwm3, I wanted to help with development, so I've been running fvwm3 in Valgrind with the VcXsrv X server on WSL. I have a virtual machine too, but it runs KDE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants