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

[1.1.x] Bugfix for buffer-overflow in cardreader #10925

Merged
merged 1 commit into from Jun 6, 2018

Conversation

akunt
Copy link

@akunt akunt commented Jun 3, 2018

Description

In CardReader::diveToFile, the calculated len (length of the directory name) is not validated before being used to strncpy the directory name to the array of fixed size dosSubdirname[FILENAME_LENGTH].

For overly long directory names, this results in a buffer-overflow which can be triggered by several SD card related G-Code instructions.

The same issue applies to the 2.0.x branch of Marlin.

Benefits

The error can be prevented by checking the resulting length of the directory name.

@thinkyhead
Copy link
Member

thinkyhead commented Jun 6, 2018

Thanks! But actually, either of these would also prevent an overflow…

Reserve one more byte:

  char dosSubdirname[FILENAME_LENGTH + 1]; // Max DOS 8.3 length (11) plus nul
  while (dirname_start) {
    . . .
  }

Reserve exactly the space needed:

    const int8_t len = dirname_end - dirname_start;
    if (len <= 0) break;
    char dosSubdirname[len + 1];
    strncpy(dosSubdirname, dirname_start, len);
    dosSubdirname[len] = 0;

@thinkyhead thinkyhead merged this pull request into MarlinFirmware:bugfix-1.1.x Jun 6, 2018
@akunt
Copy link
Author

akunt commented Jun 6, 2018

Heya!

Both of these ways should also work fine for normal printer usage.

The reason for me picking the validation method was, that the strncpy is performed regardless of whether the file actually exists on the SD card. That means if - for whatever reason - the G-Code command contains a large, non DOS 8.3 conform filename, the error is triggered. Since DOS conform filenames are also expected at other parts of the code, I assumed this would be safest way.

But the solution you merged also seems fine for me, as the directory name is validated for DOS 8.3 conformity in the call to myDir.open and that is the only further usage of dosSubdirname.

Cheers!

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

Successfully merging this pull request may close these issues.

None yet

2 participants