From 0313e4b369b7954ea01c022539f6331bdf1bb121 Mon Sep 17 00:00:00 2001 From: Mike Bryant Date: Thu, 21 Sep 2023 12:59:57 +0100 Subject: [PATCH] Fix XSS vulnerability via unsafe rendering of objects in link text We (sloppily and unnecessarily) wrapped object toStrings in Html() when rendering links. This resulted in a vulnerability where a user could put XSS in their name and it would be rendered via name links. This is fixed via a better separation between text and html content passed to the link renderering partial and by not arbitrarily assuming names are safe. Additionally, the highlighter has another method for explicitely non-tagged text which is sanitised. TODO: more auditing of the existing highlighting method, which needs to handle mixed content. Possibly server-side scrubbing of some fields. Fixes #1482 --- .../admin/common/propertyList.scala.html | 8 +++---- .../app/views/admin/helpers/linkTo.scala.html | 4 ++-- .../admin/helpers/linkToWithBody.scala.html | 4 ++-- .../helpers/linkToWithFragment.scala.html | 6 +++-- .../app/views/admin/link/itemLink.scala.html | 2 +- .../views/formHelpers/submitButton.scala.html | 2 +- .../scala/services/search/SearchHit.scala | 24 ++++++++++++++----- .../src/main/scala/views/Highlighter.scala | 4 ++++ .../scala/services/search/SearchHitSpec.scala | 6 +++++ .../views/activity/eventSummary.scala.html | 2 +- .../views/common/accessPointLink.scala.html | 2 +- .../app/views/common/accessPoints.scala.html | 2 +- .../app/views/common/otherNameList.scala.html | 2 +- .../app/views/concept/listItemBody.scala.html | 2 +- .../app/views/concept/searchItem.scala.html | 2 +- .../app/views/country/listItemMeta.scala.html | 3 +-- .../app/views/country/searchItem.scala.html | 4 ++-- .../views/documentaryUnit/infobar.scala.html | 4 ++-- .../documentaryUnit/listItemBody.scala.html | 2 +- .../documentaryUnit/searchItem.scala.html | 4 ++-- .../app/views/helpers/linkTo.scala.html | 4 ++-- .../helpers/linkToWithFragment.scala.html | 6 +++-- .../historicalAgent/listItemBody.scala.html | 2 +- .../historicalAgent/searchItem.scala.html | 2 +- .../repository/conciseAddress.scala.html | 4 ++-- .../views/repository/searchItem.scala.html | 4 ++-- .../views/userProfile/annotation.scala.html | 2 +- .../app/views/virtualUnit/infobar.scala.html | 2 +- .../views/virtualUnit/searchItem.scala.html | 6 ++--- test/integration/portal/AnnotationsSpec.scala | 1 - .../integration/portal/UserProfilesSpec.scala | 3 +-- 31 files changed, 74 insertions(+), 51 deletions(-) diff --git a/modules/admin/app/views/admin/common/propertyList.scala.html b/modules/admin/app/views/admin/common/propertyList.scala.html index ce9b68d7dc..1a712f8ccc 100644 --- a/modules/admin/app/views/admin/common/propertyList.scala.html +++ b/modules/admin/app/views/admin/common/propertyList.scala.html @@ -7,13 +7,13 @@ case s: String if s.isEmpty => { } case Some(s) => { -
  • @Html(highlighter.highlight(s.toString))
    • +
  • @highlighter.highlightText(s.toString)
    • } case s: String => { -
  • @Html(highlighter.highlight(s))
    • +
  • @highlighter.highlightText(s)
    • } case s => { -
  • @Html(highlighter.highlight(s.toString))
    • +
  • @highlighter.highlightText(s.toString)
    • } } -} \ No newline at end of file +} diff --git a/modules/admin/app/views/admin/helpers/linkTo.scala.html b/modules/admin/app/views/admin/helpers/linkTo.scala.html index 065f91f6e1..17981d03d6 100644 --- a/modules/admin/app/views/admin/helpers/linkTo.scala.html +++ b/modules/admin/app/views/admin/helpers/linkTo.scala.html @@ -1,3 +1,3 @@ -@(item: Model, content: Html = Html(""), attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(implicit userOpt: Option[UserProfile], req: RequestHeader, messages: Messages, conf: AppConfig) +@(item: Model, htmlContent: Html = Html(""), textContent: Option[String] = None, attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(implicit userOpt: Option[UserProfile], req: RequestHeader, messages: Messages, conf: AppConfig) -@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, content=content, attributes=attributes) +@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, htmlContent=htmlContent, textContent=textContent, attributes=attributes) diff --git a/modules/admin/app/views/admin/helpers/linkToWithBody.scala.html b/modules/admin/app/views/admin/helpers/linkToWithBody.scala.html index 546b829043..4f1eb82b06 100644 --- a/modules/admin/app/views/admin/helpers/linkToWithBody.scala.html +++ b/modules/admin/app/views/admin/helpers/linkToWithBody.scala.html @@ -1,3 +1,3 @@ -@(item: Model, attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(content: Html)(implicit req: RequestHeader, messages: Messages) +@(item: Model, attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(htmlContent: Html)(implicit req: RequestHeader, messages: Messages) -@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, content=content, attributes=attributes) +@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, htmlContent=htmlContent, attributes=attributes) diff --git a/modules/admin/app/views/admin/helpers/linkToWithFragment.scala.html b/modules/admin/app/views/admin/helpers/linkToWithFragment.scala.html index da7a2c3954..5e5141d3c2 100644 --- a/modules/admin/app/views/admin/helpers/linkToWithFragment.scala.html +++ b/modules/admin/app/views/admin/helpers/linkToWithFragment.scala.html @@ -1,3 +1,5 @@ -@(item: Model, fragment: String, content: Html = Html(""), attributes: Seq[(Symbol,String)] = Seq.empty)(implicit req: RequestHeader, messages: Messages) +@(item: Model, fragment: String, htmlContent: Html = Html(""), textContent: Option[String] = None, attributes: Seq[(Symbol,String)] = Seq.empty)(implicit req: RequestHeader, messages: Messages) -@{if(content.body.trim.isEmpty) Html(item.toStringLang) else content} + + @{if(htmlContent.body.trim.isEmpty) textContent.getOrElse(item.toStringLang) else htmlContent} + diff --git a/modules/admin/app/views/admin/link/itemLink.scala.html b/modules/admin/app/views/admin/link/itemLink.scala.html index b7f717df83..9677a3fafd 100644 --- a/modules/admin/app/views/admin/link/itemLink.scala.html +++ b/modules/admin/app/views/admin/link/itemLink.scala.html @@ -1,6 +1,6 @@ @(link: Link, item: Model, src: Option[Model], text: Option[String] = None)(implicit userOpt: Option[UserProfile], req: RequestHeader, conf: AppConfig, messages: Messages) -@views.html.admin.helpers.linkTo(item, content = Html(text.getOrElse(item.toStringLang))) +@views.html.admin.helpers.linkTo(item, textContent = Some(text.getOrElse(item.toStringLang))) @Messages("link." + link.data.linkType.toString) diff --git a/modules/admin/app/views/formHelpers/submitButton.scala.html b/modules/admin/app/views/formHelpers/submitButton.scala.html index 42628c63ff..6fea99230a 100644 --- a/modules/admin/app/views/formHelpers/submitButton.scala.html +++ b/modules/admin/app/views/formHelpers/submitButton.scala.html @@ -8,4 +8,4 @@ } @extra - \ No newline at end of file + diff --git a/modules/core/src/main/scala/services/search/SearchHit.scala b/modules/core/src/main/scala/services/search/SearchHit.scala index f559f6f35e..43f00fae7c 100644 --- a/modules/core/src/main/scala/services/search/SearchHit.scala +++ b/modules/core/src/main/scala/services/search/SearchHit.scala @@ -4,7 +4,9 @@ import scala.annotation.tailrec import SearchConstants._ import models.EntityType import play.api.libs.json.JsValue -import play.twirl.api.HtmlFormat +import play.twirl.api.{Html,HtmlFormat} +import org.jsoup.Jsoup +import org.jsoup.safety.Whitelist /** * Class representing a search engine hit @@ -42,18 +44,20 @@ case class SearchHit( private lazy val highlightFields: List[String] = filteredHighlights.values.flatten.toList /** - * Try and highlight some text according to the highlights. + * Try and highlight some text according to the highlights. The input text is + * assumed to already contain HTML. + * * @param text Some input text - * @return The text and a boolean indicating if highlighting was successful + * @return The text, with highlights inserted */ def highlight(text: String): String = { - def canHighlightWith(raw: String, text: String): Either[String,String] = { + def canHighlightWith(rawHighlight: String, text: String): Either[String,String] = { // NB: We have to escape the input string using HtmlFormat because it's // specialised to avoid XSS. This assumes that the highlight material coming // back from Solr doesn't itself contain XSS attacks... what are the chances // this will come back to bite me? - val stripped = HtmlFormat.escape(stripTags(raw)).body - if (text.contains(stripped)) Right(text.replace(stripped, raw)) + val stripped = HtmlFormat.escape(stripTags(rawHighlight)).body + if (text.contains(stripped)) Right(text.replace(stripped, rawHighlight)) else Left(text) } @tailrec def tryHighlight(texts: Seq[String], input: String, ok: Boolean): String = { @@ -68,4 +72,12 @@ case class SearchHit( tryHighlight(highlightFields, text, ok = false) } + + /** + * Try and highlight some text which we know should not contain HTML. + * + * @param text Some input text + * @return The text, with highlights inserted + */ + def highlightText(text: String): Html = Html(highlight(Jsoup.clean(text, Whitelist.none()))) } diff --git a/modules/core/src/main/scala/views/Highlighter.scala b/modules/core/src/main/scala/views/Highlighter.scala index 8b20b505b1..0401e5ca8a 100644 --- a/modules/core/src/main/scala/views/Highlighter.scala +++ b/modules/core/src/main/scala/views/Highlighter.scala @@ -1,9 +1,13 @@ package views +import play.twirl.api.Html + object NoopHighlighter extends Highlighter { def highlight(text: String): String = text + def highlightText(text: String): Html = Html(text) } trait Highlighter { def highlight(text: String): String + def highlightText(text: String): Html } diff --git a/modules/core/src/test/scala/services/search/SearchHitSpec.scala b/modules/core/src/test/scala/services/search/SearchHitSpec.scala index 174d68b7e6..7da077e115 100644 --- a/modules/core/src/test/scala/services/search/SearchHitSpec.scala +++ b/modules/core/src/test/scala/services/search/SearchHitSpec.scala @@ -61,5 +61,11 @@ class SearchHitSpec extends PlaySpecification { hl must contain("Demandes") hl must not contain("lu-006007-lu-11-iv-3-286") } + + "prevent XSS attacks" in { + val hl = testHit.highlightText("some text") + hl.body must not contain "" val testInstitution = "Nefarious" val data = Map( - UserProfileF.NAME -> Seq(testName), + UserProfileF.NAME -> Seq(testName + script), UserProfileF.INSTITUTION -> Seq(testInstitution + script) ) val update = FakeRequest(profileRoutes.updateProfilePost()) @@ -150,7 +150,6 @@ class UserProfilesSpec extends IntegrationTestRunner with FakeMultipartUpload { contentAsString(result) must contain(message("errors.badFileType")) } - "allow uploading image files as profile image" in new ITestApp { val file = getProfileImage val stamp = FileStorage.fingerprint(file)