MikeMcQuaid
commented
5 years ago $ brew install ./test.rb
This is not supported syntax for installing formula. What is the output of brew formula test?
Closed FiloSottile closed 5 years ago
MikeMcQuaid
commented
5 years ago $ brew install ./test.rb
This is not supported syntax for installing formula. What is the output of brew formula test?
FiloSottile
commented
5 years ago $ brew formula test
Error: No available formula with the name "test"
$ brew install ./test.rb
==> Downloading https://github.com/richfelker/musl-cross-make/archive/v0.9.8.tar.gz
==> Downloading from https://codeload.github.com/richfelker/musl-cross-make/tar.gz/v0.9.8
######################################################################## 100.0%
==> Downloading https://cdn.kernel.org/pub/linux/kernel/v4.x/linux-4.4.10.tar.xz
######################################################################## 100.0%
🍺 /usr/local/Cellar/test/0.9.8: 3 files, 6.5KB, built in 9 seconds
$ brew formula test
/usr/local/opt/test/.brew/test.rbI have definitely seen that syntax once or twice in the wild. Not saying it should be supported, but if it's unsupported it should exit with an error, or at least not silently skip critical security checks.
MikeMcQuaid
commented
5 years ago Not saying it should be supported, but if it's unsupported it should exit with an error, or at least not silently skip critical security checks.
It's not clear to me that it is. What's the full output of brew install ./test.rb --debug --verbose and brew fetch ./test.rb --debug --verbose (run multiple times in a row each, please)?
FiloSottile
commented
5 years ago brew fetch shows a "Warning", while brew install skips the "Verifying" step entirely.
As a matter of best practice and security hygiene, I would recommend checking the hash before committing the contents to cache, by the way, and to treat a mismatching hash the same way as a failed fetch. It's always safer not to let attacker controlled invalid data into the system, where it might get used by mistake by other parts of the code.
FiloSottile
commented
5 years ago It looks like this is not due to the way the formula is installed (as it reproduces identically from a tap), but to the way the resource is fetched. Using the following syntax leads to a ChecksumMismatchError as expected.
buildpath.install resource("linux-4.4.10.tar.xz")(This is indeed why I'd suggest making verification an atomic part of fetching.)
MikeMcQuaid
commented
5 years ago Ok, I see the issue here. It's with your formula:
cp resource.fetch, buildpath/resource.name
This fetches the resource but does not validate the checksum. You will notice all use of resource.fetch in Homebrew/homebrew-core are in the form r.verify_download_integrity(r.fetch). Resource#stage is the preferred API for this use-case.
As a matter of best practice and security hygiene, I would recommend checking the hash before committing the contents to cache, by the way (This is indeed why I'd suggest making verification an atomic part of fetching.)
We would review a PR to address this that also took into consideration the use-case for when users are e.g. behind a currently unconfigured proxy that we don't want to nuke their existing cache entries for already downloaded items.
It's always safer not to let attacker controlled invalid data into the system
99.9% (probably with a lot of nines missing) of the time for users experiencing checksum invalidation it's an issue with upstream, a mirror, a proxy etc. so it's unclear here that the security pay-off would outweigh the usability one.
FiloSottile
commented
5 years ago I appreciate the project, but there is no chance I have time, or am qualified, to make security relevant changes in a Ruby project, sorry.
For the sake of safety, I would strongly recommend renaming that API fetch_without_verifying or something similar. Sounds like no formula author should ever use it anyway.
when users are e.g. behind a currently unconfigured proxy that we don't want to nuke their existing cache entries for already downloaded items
Making verification atomic would make it even easier not to nuke the cache: if the checksums don't match, the contents don't make it into the cache at all, and there is no need to ever remove anything.
99.9% (probably with a lot of nines missing) of the time for users experiencing checksum invalidation it's an issue with upstream, a mirror, a proxy etc. so it's unclear here that the security pay-off would outweigh the usability one.
I don't understand this. Surely also in the "benign" 99.9% you want to reject the contents, right? You never want to install contents that don't match the upstream expectation. What I'm suggesting is rejecting them earlier, for robustness. It's just an internals change, I don't see how there would be any user impact at all.
MikeMcQuaid
commented
5 years ago I appreciate the project, but there is no chance I have time, or am qualified, to make security relevant changes in a Ruby project, sorry.
Ok.
For the sake of safety, I would strongly recommend renaming that API
fetch_without_verifyingor something similar. Sounds like no formula author should ever use it anyway.
We cannot rename it without breaking the existing, correct use of the API r.verify_download_integrity(r.fetch). We could perhaps make it verify by default and allow a parameter for disabling this in the places internally where this is necessary.
Making verification atomic would make it even easier not to nuke the cache: if the checksums don't match, the contents don't make it into the cache at all, and there is no need to ever remove anything.
I don't disagree with any of this but I'm struggling to see a clear enough attack vector to warrant the work it would take (and you're the first person to suggest/mention this). This would be a non-trivial change given how we currently handle this and the APIs we provide.
MikeMcQuaid
commented
5 years ago We cannot rename it without breaking the existing, correct use of the API
r.verify_download_integrity(r.fetch). We could perhaps make it verify by default and allow a parameter for disabling this in the places internally where this is necessary.
Reopening to address this.
brewcommand and reproduced the problem with multiple formulae? If it's a problem with a single, official formula (not cask) please file this issue at Homebrew/homebrew-core: https://github.com/Homebrew/homebrew-core/issues/new/choose. If it's abrew caskproblem please file this issue at https://github.com/Homebrew/homebrew-cask/issues/new/choose. If it's a tap (e.g. Homebrew/homebrew-php) problem please file this issue at the tap.brew updateand can still reproduce the problem?brew doctor, fixed all issues and can still reproduce the problem?brew configandbrew doctorand included their output with your issue?What you were trying to do (and why)
Installing a Formula with a wrong resource sha256.
What happened (include command output)
What you expected to happen
An error should have occurred, as the sha256 of linux-4.4.10.tar.xz is incorrect.
Step-by-step reproduction instructions (by running
brewcommands)