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.
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
Add constant time check for shared secret #2319
Conversation
8c8473f
to
665a665
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one small comment.
R/server.R
Outdated
@@ -161,7 +161,7 @@ createAppHandlers <- function(httpHandlers, serverFuncSource) { | |||
# This value, if non-NULL, must be present on all HTTP and WebSocket | |||
# requests as the Shiny-Shared-Secret header or else access will be | |||
# denied (403 response for HTTP, and instant close for websocket). | |||
sharedSecret <- getOption('shiny.sharedSecret') | |||
sharedSecret <- loadSharedSecret() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A name like checkSharedSecret
(as is used in the tests) would be a little clearer.
R/utils.R
Outdated
constantTimeEquals <- function(raw1, raw2) { | ||
stopifnot(is.raw(raw1)) | ||
stopifnot(is.raw(raw2)) | ||
stopifnot(length(raw1) == length(raw2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter that this could reveal the length using a timing attack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion, we're not going to digest the input.
- Rename sharedSecret variables to checkSharedSecret - Don't perform the digest::digest(). This just means the timing could give away the length of the secret, but that's OK, there's enough entropy in the secret even if you know its length.
6c86fd6
to
59dd4b0
Compare
@wch Can you review this last commit? Thanks! |
Here's an exaggerated test to show that comparing two raw vectors using
identical
isn't constant-time, but usingconstantTimeEquals()
is:Testing notes
Should verify that:
https://localhost:port
; this should fail.