From 9ce55146c8f830cb6bc13db2dea67bd9ee73127b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= Date: Sun, 22 Aug 2021 13:58:46 +0200 Subject: [PATCH] fix: web: b64 encode user controlled input (#1525) This fixes a reported Stored XSS vulnerability in toBloodhoundJson by encoding the user-controlled values in this payload into base64 and parsing them with atob. In my exploration of the vulnerability with various payloads I and others crafted, it would appear that this is the only available XSS in hledger-web in relation to stored accounts and transaction details. If there is other parts of the UI which may contain user-controlled data, they should be examined for similar things. In this instance, protections provided by yesod and other libraries worked fine, but in a bit of code that hledger-web was generating, the user could insert a tag (which is valid HTML and equivalent to but not caught by the T.Replace that existed in toBloodhoundJson) in order to switch out of a script context, allowing the parser to be reset, and for arbitrary JavaScript to run. The real fix is a bit more involved, but produces much better results: Content-Security-Policy headers should be introduced, and using sha256- or a different algorithm, they should be marked as trusted in the header. This way, if the (in-browser) parser and hledger-web generator disagree on the source code of the script, the script won't run. Note that this would still be susceptible to attacks that involve changing the script by escaping from the string inside it or something similar to that, which can be avoided additionally by using either the method used in this commit, or a proper JSON encoder. The second approach has the advantage of preventing further XSS, to the extent specified above, in practice, a combination of both should be used, b64 for embedded data and the CSP sha256-hash script-src over everything else, which will eliminate all injected or malformed script blocks (via CSP), in combination with eliminating any HTML closing tags which might occur in stored data (via b64). This vulnerability appears to have been first introduced when autocompletion was added in hledger-web, git tag hledger-0.24, commit hash: ec51d28839b2910eea360b1b8c72904b51cf7821 Test payload: Closes #1525 --- hledger-web/Hledger/Web/Widget/AddForm.hs | 13 ++++++++++--- hledger-web/hledger-web.cabal | 1 + hledger-web/package.yaml | 1 + stack8.6.yaml | 2 ++ stack8.8.yaml | 1 + 5 files changed, 15 insertions(+), 3 deletions(-) diff --git a/hledger-web/Hledger/Web/Widget/AddForm.hs b/hledger-web/Hledger/Web/Widget/AddForm.hs index ad31ef961..2ebc3a533 100644 --- a/hledger-web/Hledger/Web/Widget/AddForm.hs +++ b/hledger-web/Hledger/Web/Widget/AddForm.hs @@ -17,6 +17,7 @@ import Data.List (dropWhileEnd, intercalate, unfoldr) import Data.Maybe (isJust) import qualified Data.Set as S import Data.Text (Text) +import Data.Text.Encoding.Base64 (encodeBase64) import qualified Data.Text as T import Data.Time (Day) import Text.Blaze.Internal (Markup, preEscapedString) @@ -95,13 +96,19 @@ addForm j today = identifyForm "add" $ \extra -> do intercalate "," $ map ( ("{\"value\":" ++). (++"}"). - show . - -- avoid https://github.com/simonmichael/hledger/issues/236 - T.replace "" "<\\/script>" + -- This will convert a value such as ``hledger!`` into + -- ``atob("aGxlZGdlciE=")``. When this gets evaluated on the client, + -- the resulting string is ``hledger!`` again. The same data is + -- passed, but the user-controlled bit of that string can only use + -- characters [a-zA-Z0-9+=/], making it impossible to break out of + -- string context. + b64wrap ) ts, "]" ] where +b64wrap :: Text -> String +b64wrap = ("atob(\""++) . (++"\")") . T.unpack . encodeBase64 validateTransaction :: FormResult Day diff --git a/hledger-web/hledger-web.cabal b/hledger-web/hledger-web.cabal index b79d34df7..ac1641fcb 100644 --- a/hledger-web/hledger-web.cabal +++ b/hledger-web/hledger-web.cabal @@ -156,6 +156,7 @@ library Decimal >=0.5.1 , aeson >=1 , base >=4.11 && <4.16 + , base64 , blaze-html , blaze-markup , bytestring diff --git a/hledger-web/package.yaml b/hledger-web/package.yaml index 6ee1a52cd..d9e395b8e 100644 --- a/hledger-web/package.yaml +++ b/hledger-web/package.yaml @@ -103,6 +103,7 @@ library: - hledger >=1.22.99 && <1.23 - aeson >=1 - base >=4.11 && <4.16 + - base64 - blaze-html - blaze-markup - bytestring diff --git a/stack8.6.yaml b/stack8.6.yaml index 816f916eb..fc5b6cde0 100644 --- a/stack8.6.yaml +++ b/stack8.6.yaml @@ -31,6 +31,8 @@ extra-deps: - githash-0.1.4.0 # for hledger-ui: # for hledger-web: +- ghc-byteorder-4.11.0.0.10 +- base64-0.4.2.3 # Workaround for https://github.com/commercialhaskell/stack/issues/3922 # Try dropping this.. after stack 2 has been out a while ? Or now ? How about now ? diff --git a/stack8.8.yaml b/stack8.8.yaml index 140554d53..39e1d8bd3 100644 --- a/stack8.8.yaml +++ b/stack8.8.yaml @@ -20,6 +20,7 @@ extra-deps: # for hledger: # for hledger-ui: # for hledger-web: +- ghc-byteorder-4.11.0.0.10 # for Shake.hs: # for precise profiling, per https://www.tweag.io/posts/2020-01-30-haskell-profiling.html: