Skip to content

Commit

Permalink
fix: mitigate "sequential @import chaining" vulnerability (#5616)
Browse files Browse the repository at this point in the history
* `test/e2e-cypress/tests/features/xss/` -> `test/e2e-cypress/tests/security`

* add tests

* filter <style> tags out of Markdown fields

* initialize OAuth inputs without applying `value` attribute
  • Loading branch information
shockey authored Sep 20, 2019
1 parent c8ad396 commit 5f6ec8c
Show file tree
Hide file tree
Showing 11 changed files with 143 additions and 7 deletions.
9 changes: 5 additions & 4 deletions src/core/components/auth/oauth2.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export default class Oauth2 extends React.Component {
const AuthError = getComponent("authError")
const JumpToPath = getComponent("JumpToPath", true)
const Markdown = getComponent( "Markdown" )
const InitializedInput = getComponent("InitializedInput")

const { isOAS3 } = specSelectors

Expand Down Expand Up @@ -170,10 +171,10 @@ export default class Oauth2 extends React.Component {
{
isAuthorized ? <code> ****** </code>
: <Col tablet={10} desktop={10}>
<input id="client_id"
<InitializedInput id="client_id"
type="text"
required={ flow === PASSWORD }
value={ this.state.clientId }
initialValue={ this.state.clientId }
data-name="clientId"
onChange={ this.onInputChange }/>
</Col>
Expand All @@ -187,8 +188,8 @@ export default class Oauth2 extends React.Component {
{
isAuthorized ? <code> ****** </code>
: <Col tablet={10} desktop={10}>
<input id="client_secret"
value={ this.state.clientSecret }
<InitializedInput id="client_secret"
initialValue={ this.state.clientSecret }
type="text"
data-name="clientSecret"
onChange={ this.onInputChange }/>
Expand Down
36 changes: 36 additions & 0 deletions src/core/components/initialized-input.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// This component provides an interface that feels like an uncontrolled input
// to consumers, while providing a `defaultValue` interface that initializes
// the input's value using JavaScript value property APIs instead of React's
// vanilla[0] implementation that uses HTML value attributes.
//
// This is useful in situations where we don't want to surface an input's value
// into the HTML/CSS-exposed side of the DOM, for example to avoid sequential
// input chaining attacks[1].
//
// [0]: https://github.com/facebook/react/blob/baff5cc2f69d30589a5dc65b089e47765437294b/fixtures/dom/src/components/fixtures/text-inputs/README.md
// [1]: https://github.com/d0nutptr/sic

import React from "react"
import PropTypes from "prop-types"

export default class InitializedInput extends React.Component {
componentDidMount() {
// Set the element's `value` property (*not* the `value` attribute)
// once, on mount, if an `initialValue` is provided.
if(this.props.initialValue) {
this.inputRef.value = this.props.initialValue
}
}

render() {
// Filter out `value` and `defaultValue`, since we have our own
// `initialValue` interface that we provide.
// eslint-disable-next-line no-unused-vars, react/prop-types
const { value, defaultValue, ...otherProps } = this.props
return <input {...otherProps} ref={c => this.inputRef = c} />
}
}

InitializedInput.propTypes = {
initialValue: PropTypes.string
}
3 changes: 2 additions & 1 deletion src/core/components/providers/markdown.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export default Markdown

export function sanitizer(str) {
return DomPurify.sanitize(str, {
ADD_ATTR: ["target"]
ADD_ATTR: ["target"],
FORBID_TAGS: ["style"],
})
}
2 changes: 2 additions & 0 deletions src/core/presets/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import Headers from "core/components/headers"
import Errors from "core/components/errors"
import ContentType from "core/components/content-type"
import Overview from "core/components/overview"
import InitializedInput from "core/components/initialized-input"
import Info, {
InfoUrl,
InfoBasePath
Expand Down Expand Up @@ -105,6 +106,7 @@ export default function() {
basicAuth: BasicAuth,
clear: Clear,
liveResponse: LiveResponse,
InitializedInput,
info: Info,
InfoContainer,
JumpToPath,
Expand Down
13 changes: 12 additions & 1 deletion test/e2e-cypress/static/documents/petstore-expanded.openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ info:
url: https://www.apache.org/licenses/LICENSE-2.0.html
servers:
- url: http://petstore.swagger.io/api
security:
- Petstore: []
paths:
/pets:
get:
Expand Down Expand Up @@ -152,4 +154,13 @@ components:
type: integer
format: int32
message:
type: string
type: string
securitySchemes:
Petstore:
type: oauth2
flows:
implicit:
authorizationUrl: https://example.com/api/oauth/dialog
scopes:
write:pets: modify pets in your account
read:pets: read your pets
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
* {
color: red !important; /* for humans */
}

h4 {
display: none; /* for machines, used to trace whether this sheet is applied */
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
openapi: "3.0.0"

info:
title: Sequential Import Chaining
description: >
<h4>This h4 would be hidden by the injected CSS</h4>
This document tests the ability of a `<style>` tag in a Markdown field to pull in a remote stylesheet using an `@import` directive.
<style>@import url(/documents/security/sequential-import-chaining/injection.css);</style>
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
swagger: "2.0"

info:
title: Sequential Import Chaining
description: >
<h4>This h4 would be hidden by the injected CSS</h4>
This document tests the ability of a `<style>` tag in a Markdown field to pull in a remote stylesheet using an `@import` directive.
<style>@import url(/documents/security/sequential-import-chaining/injection.css);</style>
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
describe("XSS: OAuth2 authorizationUrl sanitization", () => {
it("should filter out a javascript URL", () => {
cy.visit("/?url=/documents/xss/oauth2.yaml")
cy.visit("/?url=/documents/security/xss-oauth2.yaml")
.window()
.then(win => {
let args = null
Expand Down
58 changes: 58 additions & 0 deletions test/e2e-cypress/tests/security/sequential-import-chaining.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
describe("Security: CSS Sequential Import Chaining", () => {
describe("in OpenAPI 3.0", () => {
describe("CSS Injection via Markdown", () => {
it("should filter <style> tags out of Markdown fields", () => {
cy.visit("/?url=/documents/security/sequential-import-chaining/openapi.yaml")
.get("div.information-container")
.should("exist")
.and("not.have.descendants", "style")
})
it("should not apply `@import`ed CSS stylesheets", () => {
cy.visit("/?url=/documents/security/sequential-import-chaining/openapi.yaml")
.wait(500) // HACK: wait for CSS import to settle
.get("div.info h4")
.should("have.length", 1)
.and("not.be.hidden")
})
})
describe("Value Exfiltration via CSS", () => {
it("should not allow OAuth credentials to be visible via HTML `value` attribute", () => {
cy.visit("/?url=/documents/petstore-expanded.openapi.yaml")
.get(".scheme-container > .schemes > .auth-wrapper > .btn > span")
.click()
.get("div > div > .wrapper > .block-tablet > #client_id")
.clear()
.type("abc")
.should("not.have.attr", "value", "abc")
})
})
})
describe("in Swagger 2.0", () => {
describe("CSS Injection via Markdown", () => {
it("should filter <style> tags out of Markdown fields", () => {
cy.visit("/?url=/documents/security/sequential-import-chaining/swagger.yaml")
.get("div.information-container")
.should("exist")
.and("not.have.descendants", "style")
})
it("should not apply `@import`ed CSS stylesheets", () => {
cy.visit("/?url=/documents/security/sequential-import-chaining/swagger.yaml")
.wait(500) // HACK: wait for CSS import to settle
.get("div.info h4")
.should("have.length", 1)
.and("not.be.hidden")
})
})
describe("Value Exfiltration via CSS", () => {
it("should not allow OAuth credentials to be visible via HTML `value` attribute", () => {
cy.visit("/?url=/documents/petstore.swagger.yaml")
.get(".scheme-container > .schemes > .auth-wrapper > .btn > span")
.click()
.get("div > div > .wrapper > .block-tablet > #client_id")
.clear()
.type("abc")
.should("not.have.attr", "value", "abc")
})
})
})
})

0 comments on commit 5f6ec8c

Please # to comment.