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

Package name can potentially access all flash and ram #1147

Closed
itf opened this issue Aug 7, 2018 · 2 comments
Closed

Package name can potentially access all flash and ram #1147

itf opened this issue Aug 7, 2018 · 2 comments
Labels

Comments

@itf
Copy link
Contributor

itf commented Aug 7, 2018

I haven't written code that exploits the bug yet, so it is just a theoretical bug.

Summary

If you are using a TBF version 1, there is no check on the length of the package name, so it could be as large as all the flash memory available.

Since the package name is public, every capsule could have unrestricted read access to all memory, using only safe code.

Exploiting the bug

There are two ways of exploiting the bug:
One could create a User process using a TbfHeaderV1 with an arbitrary large package name length, and later on a Capsule could use its package name to read arbitrary memory.

Or, a capsule could call the safe function "get_package_name" with a raw pointer that points to a fake TBF version 1 header and use it to read the data anywhere by using the package name offset and length.

Code:

Inside process.rs lines 267

/// Name of the app. Public so that IPC can use it.
pub package_name: &'static str,

Now lines 590, and 704 inside the function create:

let package_name = tbf_header.get_package_name(app_flash_address);
[...]
process.package_name = package_name;

And at last, inside the function tbf_header.get_package_name

crate fn get_package_name(&self, flash_start_addr: *const u8) -> &'static str {
        match *self {
            TbfHeader::TbfHeaderV1(hd) => unsafe {
                let package_name_byte_array = slice::from_raw_parts(
                    flash_start_addr.offset(hd.pkg_name_offset as isize),
                    hd.pkg_name_size as usize,
                );
                let mut app_name_str = "";
                let _ = str::from_utf8(package_name_byte_array).map(|name_str| {
                    app_name_str = name_str;
                });
                app_name_str
            },

There are no checks for either pkg_name_offset nor pkg_name_size.

Proposed solutions

First the get_package_name should probably be made unsafe, and then either some package name size check should be placed (either in the get package name function, or in the parse_and_validate_tbf_header function) or the support for V1 header should be dropped.

@bradjc bradjc added the bug label Aug 7, 2018
@bradjc
Copy link
Contributor

bradjc commented Aug 7, 2018

We almost certainly should remove support for TBF header V1 as I can't imagine it is still used anywhere at this point.

As for the get_package_name() function, unfortunately we are going the wrong way with respect to that. #1113 changes how the process-specific code is handled, and actually makes way more functions public through the ProcessType trait (although code would still need a reference to something that actually implements that trait to call the functions). What we ideally need is some way to make the ProcessType trait public, while making all of the functions of the trait only accessible within the crate.

@alevy
Copy link
Member

alevy commented Aug 8, 2018

@itf excellent work and thank you!

This is not currently exploitable by arbitrary capsules, since get_package_name has crate scope, and (untrusted) capsules are not housed in the kernel crate.

Nonetheless, all of your critiques are still valid as misusing this function inside the kernel crate seems likely, and it seems risky to rely on it not being more visible (seems logical to expose this particular function at some point in the future in a careless attempt to make the API more useful).

We should definitely remove header V1... it's beyond deprecated. We should also do a pass on marking functions inside the kernel crate unsafe unless they expose a safe API.

bors bot added a commit that referenced this issue Aug 17, 2018
1150: kernel: remove TBF header v1 r=bradjc a=bradjc

V1 of the TBF header has been deprecated for a while now, and there is very little chance that any boards still have apps with the old tbf header version.

This also removes the PIC options field that is not supported by the kernel, not standardized, and not something we want the kernel to have to support in the future. It is also, as far as we know, unused.

~~Blocked because #1113 _needs_ to be merged.~~

Fixes #1147.


### Testing Strategy

This pull request was tested by compiling.


### TODO or Help Wanted

n/a


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
@bors bors bot closed this as completed in #1150 Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants