[Qt] Defend against a simple URI parameter pollution attack. #4050

pull aalness wants to merge 1 commits into bitcoin:master from aalness:master changing 1 files +22 −8

aalness commented at 1:53 AM on April 13, 2014: contributor

Only allow URIs which contain a single "amount" parameter.
leofidus commented at 12:33 PM on April 13, 2014: none

amount is without a doubt the most important parameter, but wouldn't it be preferable to protect all (known or unknown) parameters against pollution?
laanwj commented at 1:44 PM on April 13, 2014: member

Yes, good point about consistency. If we forbid duplicate parameters it should probably apply to all currently used ones (but not unknown/future ones, there may be a valid reason for repeating some). But I hope this will be discussed with the spec.
aalness commented at 4:55 PM on April 13, 2014: contributor

Please disregard current patch. I just realized it conflicts with @laanwj's suggestion. Will update.
aalness commented at 5:19 PM on April 13, 2014: contributor

Updated. Current patch should be consistent with BIP 21 and incorporates feedback. Thanks guys.
aalness renamed this:
~~Defend against a simple URI parameter pollution atack.~~
Defend against a simple URI parameter pollution attack.
on Apr 13, 2014
aalness commented at 6:03 PM on April 18, 2014: contributor

Tested with the following URIs:

bitcoin:15DFRJVUfdKEZygG6Kimxg9VVpRDU3nxDh?amount=1.00&message=test&amount=10.00 (fails)

bitcoin:15DFRJVUfdKEZygG6Kimxg9VVpRDU3nxDh?amount=1.00&message=test&req-amount=10.00 (fails)

bitcoin:15DFRJVUfdKEZygG6Kimxg9VVpRDU3nxDh?amount=1.00&message=test&foo=bar&foo=hehe (ok)

bitcoin:15DFRJVUfdKEZygG6Kimxg9VVpRDU3nxDh?amount=1.00&message=test (ok)

bitcoin:15DFRJVUfdKEZygG6Kimxg9VVpRDU3nxDh?amount=1.00&message=test&req-foo=bar (fails)
aalness renamed this:
~~Defend against a simple URI parameter pollution attack.~~
[qt] Defend against a simple URI parameter pollution attack.
on Apr 18, 2014
aalness renamed this:
~~[qt] Defend against a simple URI parameter pollution attack.~~
[Qt] Defend against a simple URI parameter pollution attack.
on Apr 18, 2014
Defend against simple URI parameter pollution atacks.
Only allow single instances of known URI parameters.
bcdbfdb882
BitcoinPullTester commented at 7:38 PM on April 27, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bcdbfdb882540c61c6ec7f295a37d7bdd4308952 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
aalness commented at 8:44 PM on May 2, 2014: contributor

Closing until BIP revision is resolved.
aalness closed this on May 2, 2014
DrahtBot locked this on Sep 8, 2021

Contributors