Skip to content
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

mitigate "sequential @import chaining" vulnerability #5616

Merged
merged 4 commits into from
Sep 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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")
})
})
})
})