You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(letters): encode the mailto address; fuzz the register check and the letters weekly - #29
Fixes a mailto injection in the letters, and adds fuzz targets for the register check and the letters.
The fix.letters::mailto wrote the register entry's address into the link unencoded. The register check accepts any address RFC 5322 allows, including ?, &, # and % in the local part, so an entry whose address is x?cc=attacker%40evil.example%2Cy@broker.example.com.au passes registry check and its link opened a draft with a cc to attacker@evil.example: the user's request, with their identity details, would also go to the attacker. A link is only made for a letter short enough for one (2,000 characters): none of the 161 full letters the 80 shipped entries compose fits, but 165 single-lever letters do, so the injection is reachable. Each part of the address around its @ is now percent-encoded like the subject and body. The new test a_register_address_cannot_add_fields_to_a_mailto_link uses that address; it fails on main. No release contains the bug (GhostSoap has none yet); it came with mailto in #3.
Fuzzing.
fuzz/: register_letters runs an entry file through the register check and, when it passes, composes its letters, checks the eml header block and decodes each mailto link's address, subject and body to check they are exactly the letter's, for the letter and for a short copy of it; letter_text runs the renderer and the line wrapper on any text and width. Own workspace, with a lockfile seeded from the shipped Cargo.lock so the targets fuzz the dependency versions that ship (it adds only arbitrary, jobserver and libfuzzer-sys).
fuzz.yml: every Monday and on demand, 5 minutes per target on nightly with cargo-fuzz 0.13.2, seeded from registry/au and letters/au. A crashing input is kept as a workflow artefact.
Dependabot also watches fuzz/Cargo.lock, and CONTRIBUTING says how to run a target.
Checked on Linux on this branch merged with main as of #27: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --locked (104 passed; 102 on Windows before #27), the release build and registry check --path registry/au (80 entries, 0 errors). Each target ran for 180 seconds without a crash: letter_text 2,146,088 inputs and register_letters 1,596,439. A throwaway harness ran the target's decoding check over every shipped entry: all 80 pass and their 64 short-copy links decode exactly, and it rejects the old unencoded link for an added entry whose address is x?subject=spoof@acceleon.com.au.
The register check accepts any address RFC 5322 allows, so a register entry's
address may hold '?', '&', '#' or '%'. mailto() wrote it unencoded, so an entry
such as x?cc=attacker%40evil.example%2Cy@broker.example.com.au ended the
address early and added a cc field: the user's request, with their personal
details, would also go to the attacker. Each part of the address around its
'@' is now percent-encoded like the subject and body.
register_letters runs an entry file through the register check and, when it
passes, composes its letters and checks the eml header block and the mailto
fields. letter_text runs the renderer and the line wrapper on any text and
width. CI fuzzes each for 5 minutes every Monday, seeded from registry/au and
letters/au; Dependabot covers the fuzz crate's lockfile.
Review found that the link check let an address such as
x?subject=spoof@broker.example.com.au through on the old construction, since
it only counted fields. The fuzz target now decodes the link's address,
subject and body and compares them with the letter's. A whole request is
usually over the 2,000-character link limit (none of the 161 letters the 81
shipped entries compose gets a link), so the target also checks the link of a
short copy of each letter.
The fuzz crate's lock was generated fresh, so it held newer versions of
crates the parent lock pins. Seeding it from Cargo.lock means the targets
fuzz the dependency versions that ship; it adds only the fuzzing crates.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
⚠️Resolve issues before merging
Two issues warrant attention before merging. The fuzz dependency set is not clearly reproducibly pinned for clean workflow runs, and the address encoding logic may alter valid addresses containing an '@' within a quoted local part. No uncovered complex files were reported.
Test suggestions
An address containing query, fragment, percent, or ampersand characters is safely encoded without adding mailto fields.
The mailto address, subject, and body round-trip through percent decoding to their original values.
The existing 2,000-character mailto limit remains enforced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
Fixes a mailto injection in the letters, and adds fuzz targets for the register check and the letters.
The fix.
letters::mailtowrote the register entry's address into the link unencoded. The register check accepts any address RFC 5322 allows, including?,&,#and%in the local part, so an entry whose address isx?cc=attacker%40evil.example%2Cy@broker.example.com.aupassesregistry checkand its link opened a draft with a cc toattacker@evil.example: the user's request, with their identity details, would also go to the attacker. A link is only made for a letter short enough for one (2,000 characters): none of the 161 full letters the 80 shipped entries compose fits, but 165 single-lever letters do, so the injection is reachable. Each part of the address around its@is now percent-encoded like the subject and body. The new testa_register_address_cannot_add_fields_to_a_mailto_linkuses that address; it fails on main. No release contains the bug (GhostSoap has none yet); it came withmailtoin #3.Fuzzing.
fuzz/:register_lettersruns an entry file through the register check and, when it passes, composes its letters, checks the eml header block and decodes each mailto link's address, subject and body to check they are exactly the letter's, for the letter and for a short copy of it;letter_textruns the renderer and the line wrapper on any text and width. Own workspace, with a lockfile seeded from the shippedCargo.lockso the targets fuzz the dependency versions that ship (it adds onlyarbitrary,jobserverandlibfuzzer-sys).fuzz.yml: every Monday and on demand, 5 minutes per target on nightly with cargo-fuzz 0.13.2, seeded fromregistry/auandletters/au. A crashing input is kept as a workflow artefact.fuzz/Cargo.lock, and CONTRIBUTING says how to run a target.Checked on Linux on this branch merged with main as of #27:
cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test --locked(104 passed; 102 on Windows before #27), the release build andregistry check --path registry/au(80 entries, 0 errors). Each target ran for 180 seconds without a crash:letter_text2,146,088 inputs andregister_letters1,596,439. A throwaway harness ran the target's decoding check over every shipped entry: all 80 pass and their 64 short-copy links decode exactly, and it rejects the old unencoded link for an added entry whose address isx?subject=spoof@acceleon.com.au.Alcuin approved it with no findings.