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

Invalid Array Length Resulting in Stack Corruption #149

Open
Caleb-With-Lockheed opened this issue Aug 12, 2024 · 0 comments
Open

Invalid Array Length Resulting in Stack Corruption #149

Caleb-With-Lockheed opened this issue Aug 12, 2024 · 0 comments

Comments

@Caleb-With-Lockheed
Copy link

Location
File: elf2cfetbl.c
Function: GetSectionHeader()
Code:
Approximate location: line 1680

    int32  Status      = SUCCESS;
    size_t NumHdrsRead = 0;
    char   VerboseStr[60];
    int64  SeekOffset;
    int32  Shentsize;
    int32  i = 0;

Approximate location: line 1740

        while ((VerboseStr[i] = fgetc(SrcFileDesc)) != '\0')
        {
            i++;
        }

Description
With larger section header names in object files, the elf2cfe’s function: GetSectionHeader() can corrupt the stack memory. Section header names are statically allocated 60 characters at the start of the function, but the actual limit is likely MAX_SECTION_HDR_NAME_LEN.
I noted that when the section header name was larger, the Status variable was corrupted and returned nonsensical values that weren’t explicitly being set anywhere.

To Reproduce
Compile and use a table source with sufficiently large section header names(greater than 60 characters). For our use case, there was an arrangement of templates that resulted in larger section header names.

The Bug
When reading characters from the elf file into VerboseStr, there is no upper bound constraint, causing invalid memory writes after the end of VerboseStr on the stack. When executed, this can result in invalid return codes and immediate/quiet termination of the program.
Note the loop near line 1740 doesn't appear to protect from overloading VerboseStr past its capacity.

Fix
The immediate fix is a one-liner changing VerboseStr's size from the literal "60" to "MAX_SECTION_HDR_NAME_LEN", but it's worth noting there are other stack string allocations using the literal "60" and they should all be scrubbed if coding standards disapprove the use of "magic constants."

    int32  Status      = SUCCESS;
    size_t NumHdrsRead = 0;
    char   VerboseStr[**MAX_SECTION_HDR_NAME_LEN**];
    int64  SeekOffset;
    int32  Shentsize;
    int32  i = 0;

Additionally, there should be some functionally guaranteed protection against overloading VerboseStr, so the snippet of the while loop reading from a file may need additional constraints.

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

No branches or pull requests

1 participant