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

rimage: insecure data handling #258

Closed
plbossart opened this issue Aug 23, 2018 · 3 comments
Closed

rimage: insecure data handling #258

plbossart opened this issue Aug 23, 2018 · 3 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@plbossart
Copy link
Member

Coverity reports the following issue

46        /* read in sections */
   	3. tainted_data_argument: Calling function fread taints argument section.
 47        count = fread(section, sizeof(Elf32_Shdr), hdr->e_shnum, module->fd);
   	4. Condition count != hdr->e_shnum, taking false branch.
 48        if (count != hdr->e_shnum) {
 49                fprintf(stderr, "error: failed to read %s section header %d\n",
 50                        module->elf_file, -errno);
 51                return -errno;
 52        }
 53
 54        /* read in strings */
   	
CID 313455 (#1 of 2): Untrusted value as argument (TAINTED_SCALAR)
5. tainted_data: Passing tainted variable section[hdr->e_shstrndx].sh_size to a tainted sink.
 55        module->strings = calloc(1, section[hdr->e_shstrndx].sh_size);

313455 Untrusted value as argument
The argument could be controlled by an attacker, who could invoke the function with arbitrary values (for example, a very high or negative buffer size).

In elf_read_sections: An unscrutinized value from an untrusted source used as argument to a function (for example, a buffer size) (CWE-20)

@xiulipan xiulipan self-assigned this Aug 24, 2018
@mengdonglin mengdonglin added this to the TBD milestone Aug 31, 2018
@mengdonglin mengdonglin added the enhancement New feature or request label Sep 17, 2018
@tlauda
Copy link
Contributor

tlauda commented Nov 28, 2018

@plbossart section[hdr->e_shstrndx].sh_size is typedefed as uint32_t, so it cannot be negative nor we shouldn't be bothered with max value. What do you propose to add here? I feel, that checking against negative or zero values is a little bit overkill.

@tlauda
Copy link
Contributor

tlauda commented Jun 6, 2019

@xiulipan Closing as non issue. You can reopen if Coverity will still complain.

@tlauda tlauda closed this as completed Jun 6, 2019
@jajanusz
Copy link
Contributor

jajanusz commented Jun 6, 2019

@plbossart @tlauda May be marked as false positive in Coverity. Someone can hack elf section header to have big value 4 fun and it will only result in calloc failing to allocate memory, or crashing rimage. Non-security issue. Doing sanity checks for things like this will only make code less readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants