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.

Sign up for GitHub

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

Jump to bottom

Improve signature verification safeguards #8959

Merged
merged 6 commits into from
Oct 11, 2018
Merged

Improve signature verification safeguards #8959

merged 6 commits into from
Oct 11, 2018

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Oct 11, 2018

  • Enforce a 12 hours time window on signed requests to prevent replayability of captured requests. Low importance, since HTTPS normally prevents MITM attacks that would allow capturing requests.
  • Ensure that the list of signed headers in the signature header is downcased. A differently-cased list could allow the sender to side-step the special-case verification of the Digest header. Low importance, since HTTPS normally prevents MITM attacks that would allow tampering with the request.

ClearlyClaire and others added 5 commits October 11, 2018 23:25
@ClearlyClaire @Gargron
Downcase signed_headers string before building the signed string
8de6f9b 
The HTTP Signatures draft does not mandate the “headers” field to be downcased,
but mandates the header field names to be downcased in the signed string, which
means that prior to this patch, Mastodon could fail to process signatures from
some compliant clients. It also means that it would not actually check the
Digest of non-compliant clients that wouldn't use a lowercased Digest field
name.

Thankfully, I don't know of any such client.
@ClearlyClaire @Gargron
Revert "Remove dead code (#8919)"
9a117ab 
This reverts commit a00ce8c.
@ClearlyClaire @Gargron
Restore time window checking, change it to 12 hours
64f6d2f 
By checking the Date header, we can prevent replaying old vulnerable
signatures. The focus is to prevent replaying old vulnerable requests
from software that has been fixed in the meantime, so a somewhat long
window should be fine and accounts for timezone misconfiguration.
@ClearlyClaire @Gargron
Escape users' URLs when formatting them
1a3b154 
Fixes possible HTML injection
@Gargron
Escape all string interpolations in Formatter class
4179e01 
Slightly improve performance by reducing class allocations
from repeated Formatter#encode calls
@Gargron Gargron added the security Security issues and fixes, vulnerabilities label Oct 11, 2018
@Gargron Gargron merged commit 21ad21c into master Oct 11, 2018
@Gargron Gargron deleted the fix-security-improvements branch October 11, 2018 22:15
Gargron added a commit that referenced this pull request Oct 11, 2018
@Gargron
Improve signature verification safeguards (#8959)
1787704 
* Downcase signed_headers string before building the signed string

The HTTP Signatures draft does not mandate the “headers” field to be downcased,
but mandates the header field names to be downcased in the signed string, which
means that prior to this patch, Mastodon could fail to process signatures from
some compliant clients. It also means that it would not actually check the
Digest of non-compliant clients that wouldn't use a lowercased Digest field
name.

Thankfully, I don't know of any such client.

* Revert "Remove dead code (#8919)"

This reverts commit a00ce8c.

* Restore time window checking, change it to 12 hours

By checking the Date header, we can prevent replaying old vulnerable
signatures. The focus is to prevent replaying old vulnerable requests
from software that has been fixed in the meantime, so a somewhat long
window should be fine and accounts for timezone misconfiguration.

* Escape users' URLs when formatting them

Fixes possible HTML injection

* Escape all string interpolations in Formatter class

Slightly improve performance by reducing class allocations
from repeated Formatter#encode calls

* Fix code style issues
rtucker pushed a commit to vulpineclub/mastodon that referenced this pull request Oct 12, 2018
@Gargron @rtucker
Improve signature verification safeguards (mastodon#8959)
e3adb6a 
* Downcase signed_headers string before building the signed string

The HTTP Signatures draft does not mandate the “headers” field to be downcased,
but mandates the header field names to be downcased in the signed string, which
means that prior to this patch, Mastodon could fail to process signatures from
some compliant clients. It also means that it would not actually check the
Digest of non-compliant clients that wouldn't use a lowercased Digest field
name.

Thankfully, I don't know of any such client.

* Revert "Remove dead code (mastodon#8919)"

This reverts commit a00ce8c.

* Restore time window checking, change it to 12 hours

By checking the Date header, we can prevent replaying old vulnerable
signatures. The focus is to prevent replaying old vulnerable requests
from software that has been fixed in the meantime, so a somewhat long
window should be fine and accounts for timezone misconfiguration.

* Escape users' URLs when formatting them

Fixes possible HTML injection

* Escape all string interpolations in Formatter class

Slightly improve performance by reducing class allocations
from repeated Formatter#encode calls

* Fix code style issues
abcang pushed a commit to pixiv/mastodon that referenced this pull request Oct 12, 2018
@Gargron @abcang
Improve signature verification safeguards (mastodon#8959)
6576978 
* Downcase signed_headers string before building the signed string

The HTTP Signatures draft does not mandate the “headers” field to be downcased,
but mandates the header field names to be downcased in the signed string, which
means that prior to this patch, Mastodon could fail to process signatures from
some compliant clients. It also means that it would not actually check the
Digest of non-compliant clients that wouldn't use a lowercased Digest field
name.

Thankfully, I don't know of any such client.

* Revert "Remove dead code (mastodon#8919)"

This reverts commit a00ce8c.

* Restore time window checking, change it to 12 hours

By checking the Date header, we can prevent replaying old vulnerable
signatures. The focus is to prevent replaying old vulnerable requests
from software that has been fixed in the meantime, so a somewhat long
window should be fine and accounts for timezone misconfiguration.

* Escape users' URLs when formatting them

Fixes possible HTML injection

* Escape all string interpolations in Formatter class

Slightly improve performance by reducing class allocations
from repeated Formatter#encode calls

* Fix code style issues
abcang added a commit to pixiv/mastodon that referenced this pull request Oct 12, 2018
@abcang
Merge pull request mastodon#863 from pixiv/cherry_pick_2_5_1
98393dc 
Improve signature verification safeguards (mastodon#8959)
abcang pushed a commit to pixiv/mastodon that referenced this pull request Oct 12, 2018
@Gargron @abcang
Improve signature verification safeguards (mastodon#8959)
64c53f3 
* Downcase signed_headers string before building the signed string

The HTTP Signatures draft does not mandate the “headers” field to be downcased,
but mandates the header field names to be downcased in the signed string, which
means that prior to this patch, Mastodon could fail to process signatures from
some compliant clients. It also means that it would not actually check the
Digest of non-compliant clients that wouldn't use a lowercased Digest field
name.

Thankfully, I don't know of any such client.

* Revert "Remove dead code (mastodon#8919)"

This reverts commit a00ce8c.

* Restore time window checking, change it to 12 hours

By checking the Date header, we can prevent replaying old vulnerable
signatures. The focus is to prevent replaying old vulnerable requests
from software that has been fixed in the meantime, so a somewhat long
window should be fine and accounts for timezone misconfiguration.

* Escape users' URLs when formatting them

Fixes possible HTML injection

* Escape all string interpolations in Formatter class

Slightly improve performance by reducing class allocations
from repeated Formatter#encode calls

* Fix code style issues
abcang added a commit to pixiv/mastodon that referenced this pull request Oct 12, 2018
@abcang
Merge pull request mastodon#1221 from pixiv/cherry_pick_2_5_1
228f63f 
Improve signature verification safeguards (mastodon#8959)
ClearlyClaire pushed a commit to glitch-soc/mastodon that referenced this pull request Oct 12, 2018
@Gargron @ClearlyClaire
Improve signature verification safeguards (mastodon#8959)
fac5299 
* Downcase signed_headers string before building the signed string

The HTTP Signatures draft does not mandate the “headers” field to be downcased,
but mandates the header field names to be downcased in the signed string, which
means that prior to this patch, Mastodon could fail to process signatures from
some compliant clients. It also means that it would not actually check the
Digest of non-compliant clients that wouldn't use a lowercased Digest field
name.

Thankfully, I don't know of any such client.

* Revert "Remove dead code (mastodon#8919)"

This reverts commit a00ce8c.

* Restore time window checking, change it to 12 hours

By checking the Date header, we can prevent replaying old vulnerable
signatures. The focus is to prevent replaying old vulnerable requests
from software that has been fixed in the meantime, so a somewhat long
window should be fine and accounts for timezone misconfiguration.

* Escape users' URLs when formatting them

Fixes possible HTML injection

* Escape all string interpolations in Formatter class

Slightly improve performance by reducing class allocations
from repeated Formatter#encode calls

* Fix code style issues
hellekin
hellekin reviewed Oct 16, 2018
View reviewed changes
spec/controllers/concerns/signature_verification_spec.rb
@@ -73,6 +73,30 @@ def alternative_success
end
end

context 'with request older than a day' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#matches_time_window? uses 12.hours.ago, spec mentions "older than a day", and the spec itself sets the header to 2.days.ago.

lindwurm added a commit to akane-blue/mastodon that referenced this pull request Nov 1, 2018
@lindwurm
Revert "Improve signature verification safeguards (mastodon#8959)"
fccd3b1 
This reverts commit 1787704.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellekin hellekin hellekin left review comments

Assignees
No one assigned
Labels
security Security issues and fixes, vulnerabilities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gargron @hellekin @ClearlyClaire