Code review feedback

- 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.
This commit is contained in:
Joe Cheng
2019-02-05 14:19:37 -08:00
parent d73c91d4a7
commit 59dd4b0721
3 changed files with 21 additions and 8 deletions

View File

@@ -328,13 +328,13 @@ HandlerManager <- R6Class("HandlerManager",
}
)
},
.httpServer = function(handler, sharedSecret) {
.httpServer = function(handler, checkSharedSecret) {
filter <- getOption('shiny.http.response.filter')
if (is.null(filter))
filter <- function(req, response) response
function(req) {
if (!sharedSecret(req$HTTP_SHINY_SHARED_SECRET)) {
if (!checkSharedSecret(req$HTTP_SHINY_SHARED_SECRET)) {
return(list(status=403,
body='<h1>403 Forbidden</h1><p>Shared secret mismatch</p>',
headers=list('Content-Type' = 'text/html')))

View File

@@ -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 <- loadSharedSecret()
checkSharedSecret <- loadSharedSecret()
appHandlers <- list(
http = joinHandlers(c(
@@ -170,7 +170,7 @@ createAppHandlers <- function(httpHandlers, serverFuncSource) {
reactLogHandler
)),
ws = function(ws) {
if (!sharedSecret(ws$request$HTTP_SHINY_SHARED_SECRET)) {
if (!checkSharedSecret(ws$request$HTTP_SHINY_SHARED_SECRET)) {
ws$close()
return(TRUE)
}

View File

@@ -1744,14 +1744,25 @@ getSliderType <- function(min, max, value) {
# Reads the `shiny.sharedSecret` global option, and returns a function that can
# be used to test header values for a match.
loadSharedSecret <- function() {
sharedSecret <- getOption("shiny.sharedSecret")
normalizeToRaw <- function(value, label = "value") {
if (is.null(value)) {
raw()
} else if (is.character(value)) {
charToRaw(paste(value, collapse = "\n"))
} else if (is.raw(value)) {
value
} else {
stop("Wrong type for ", label, "; character or raw expected")
}
}
sharedSecret <- normalizeToRaw(getOption("shiny.sharedSecret"))
if (is.null(sharedSecret)) {
function(x) TRUE
} else {
# We compare the digest of the two values so that their lengths are equalized
sharedSecret <- digest::digest(sharedSecret, "sha256", serialize = FALSE, raw = TRUE)
function(x) {
x <- digest::digest(x, "sha256", serialize = FALSE, raw = TRUE)
x <- normalizeToRaw(x)
# Constant time comparison to avoid timing attacks
constantTimeEquals(sharedSecret, x)
}
@@ -1762,7 +1773,9 @@ loadSharedSecret <- function() {
constantTimeEquals <- function(raw1, raw2) {
stopifnot(is.raw(raw1))
stopifnot(is.raw(raw2))
stopifnot(length(raw1) == length(raw2))
if (length(raw1) != length(raw2)) {
return(FALSE)
}
sum(as.integer(xor(raw1, raw2))) == 0
}