search

mun-lang / mun

Source code for the Mun language and runtime.
https://mun-lang.org
Other
1.87k stars 73 forks source link

bump: update libloading requirement from 0.6 to 0.7 #401

Closed dependabot[bot] closed 2 years ago

dependabot[bot] commented 2 years ago

Updates the requirements on libloading to permit the latest version.

Commits
  • 00c21d8 Replace docsrs cfg with libloading_docs
  • b706286 Build wasm32-unknown-unknown in CI
  • 4b77b7b Fix documentation building on Windows
  • ec04e8d Fix broken rustdoc links
  • 840ca50 Changelog for 0.7.2
  • 4ece71d Library & Symbol exist on supported platforms only
  • e38bb29 Annotate the MSRV in Cargo.toml
  • e0ebb7b Small typo
  • c82a500 Bump 0.7.1
  • 2173d71 Removed unclear sentence
  • Additional commits viewable in compare view


Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
baszalmstra commented 2 years ago

@Wodann loading a library (Library::new) has become unsafe. I've reflected that in our use as well, but I assume that loading a munlib is safe. Would you agree with this approach or should this unsafety bubble up? I didn't think so because we also catch a few other unsafe actions in the code path.

codecov[bot] commented 2 years ago

Codecov Report

Merging #401 (8a4814d) into main (bd7466f) will decrease coverage by 0.00%. The diff coverage is 84.37%.

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
- Coverage   82.69%   82.68%   -0.01%     
==========================================
  Files         264      264              
  Lines       15232    15239       +7     
==========================================
+ Hits        12596    12601       +5     
- Misses       2636     2638       +2
Impacted Files Coverage Δ
crates/mun/src/ops/start.rs 0.00% <0.00%> (ø)
crates/mun_runtime/benches/util/mod.rs 0.00% <0.00%> (ø)
crates/mun_libloader/src/lib.rs 94.44% <88.88%> (-5.56%) :arrow_down:
crates/mun/tests/integration.rs 100.00% <100.00%> (ø)
crates/mun_codegen/tests/abi.rs 90.41% <100.00%> (ø)
crates/mun_libloader/src/temp_library.rs 100.00% <100.00%> (ø)
crates/mun_runtime/src/assembly.rs 97.91% <100.00%> (ø)
crates/mun_runtime/src/lib.rs 68.65% <100.00%> (ø)
crates/mun_skeptic/src/runtime.rs 84.61% <100.00%> (+0.61%) :arrow_up:
crates/mun_test/src/driver.rs 85.00% <100.00%> (+0.51%) :arrow_up:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bd7466f...8a4814d. Read the comment docs.

Wodann commented 2 years ago

@Wodann loading a library (Library::new) has become unsafe. I've reflected that in our use as well, but I assume that loading a munlib is safe. Would you agree with this approach or should this unsafety bubble up? I didn't think so because we also catch a few other unsafe actions in the code path.

I looked at the safety information about new:

When a library is loaded, initialisation routines contained within it are executed. For the purposes of safety, the execution of these routines is conceptually the same calling an unknown foreign function and may impose arbitrary requirements on the caller for the call to be sound.

Additionally, the callers of this function must also ensure that execution of the termination routines contained within the library is safe as well. These routines may be executed when the library is unloaded.

When a user calls add_assembly with library_path, its possible that they're providing a path to any executable. We cannot guarantee that it's a Munlib compiled by us. Does that make it unsafe? Should the user be notified that they should only trust libraries generated by the Mun compiler?

I wondered whether this would be considered a usage of the unsafe keyword:

The unsafe keyword has two uses: to declare the existence of contracts the compiler can’t check (unsafe fn and unsafe trait), and to declare that a programmer has checked that these contracts have been upheld (unsafe {} and unsafe impl, but also unsafe fn – see below). They are not mutually exclusive, as can be seen in unsafe fn.

I feel that the user has to be notified of a contract here. Safety: Only load libraries that were compiled by the Mun compiler. Maybe can somehow make this safer? Generate a public-private key combination where the compiler compiles something into the library that's validated upon load of the shared library? At that point we could (maybe?!) guarantee safety?

I'm not an expert on the security of this approach, but I do feel that there is a risk in just loading an arbitrary library that the user might need to be aware of.

baszalmstra commented 2 years ago

I think there are two concerns here that we want to take into account:

I think only the first is something that is covered by the unsafe keyword.

Undefined behavior

I think this is what the libloading new function covers. When loading a shared object, an entry point is called (on some platforms?) which might result in undefined behavior crashing the application. I just found the changelog of libloading and the corresponding issues that ensued the change where this problem is pretty well explained.

I think this affects us in the exact same manner, we cannot guarantee that a shared object is not "evil" so in that regard our functions should also be unsafe.

Security

The problem with security in Mun is that currently, we can only know anything about a Mun library after it has been loaded by libloading. If an attacker runs code in the initialization routine of the shared object there is currently nothing we can do about it.

I don't see how we can embed a private key in the compiler since Mun is open source. Even if we create a custom binary which includes the private key an attack is theoretically able to retrieve the private key from the binary itself. An attacker could use the same private key to create a custom-signed binary. This means that a developer would have to have his own private key with which he signs his binaries. I see a few possible options here:

I feel like we have some pretty decent options to figure out this security thing, its not something for now but it is something we have to properly tackle in the future. For now I'll make the loading functions unsafe as well.

baszalmstra commented 2 years ago

@dependabot rebase

My latest changes bubble the unsafety of loading a munlib to the creation of the runtime.

dependabot[bot] commented 2 years ago

Looks like this PR has been edited by someone other than Dependabot. That means Dependabot can't rebase it - sorry!

If you're happy for Dependabot to recreate it from scratch, overwriting any edits, you can request @dependabot recreate.

baszalmstra commented 2 years ago

@Wodann This is ready for your review :)