Skip to content

Commit

Permalink
fix: do not trust integrity attribute when undeserved
Browse files Browse the repository at this point in the history 
Signed-off-by: Andres Correa Casablanca <[email protected]>
  • Loading branch information
@castarco
castarco committed Mar 31, 2024
1 parent 1221019 commit 5ae8b8e
Show file tree
Hide file tree
Showing 4 changed files with 265 additions and 56 deletions.
38 changes: 38 additions & 0 deletions @kindspells/astro-shield/e2e/e2e.test.mts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -551,4 +551,42 @@ describe('middleware (hybrid 3)', () => {
),
)
})

it('does not "validate" sri signatures for cross-origin scripts that are not in the allow list', async () => {
const response = await fetch(`${baseUrl}/injected/`)
const cspHeader = response.headers.get('content-security-policy')

assert(cspHeader !== null)
assert(cspHeader)

const scriptDirective = cspHeader
.split(/;\s*/)
.filter(directive => directive.startsWith('script-src'))[0]
assert(scriptDirective)

// This hash belongs to an allowed script that included its integrity
// attribute as well (https://code.jquery.com/jquery-3.7.1.slim.min.js).
assert(
scriptDirective.includes(
'sha256-kmHvs0B+OpCW5GVHUNjv9rOmY0IvSIRcf7zGUDTDQM8=',
),
)

// This hash belongs to an allowed script that did not include its
// integrity attribute (https://code.jquery.com/ui/1.13.2/jquery-ui.min.js).
assert(
scriptDirective.includes(
'sha256-lSjKY0/srUM9BE3dPm+c4fBo1dky2v27Gdjm2uoZaL0=',
),
)

// The MOST IMPORTANT assertionf of this test:
// This hash belongs to the script that is "injected" in the page
// (more precisely, that is not in the allow list)
assert(
!scriptDirective.includes(
'sha256-BbhdlvQf/xTY9gja0Dq3HiwQF8LaCRTXxZKRutelT44=',
),
)
})
})
127 changes: 85 additions & 42 deletions @kindspells/astro-shield/src/core.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const generateSRIHash = data => {

/**
* @typedef {(
* hash: string | null,
* hash: string,
* attrs: string,
* setCrossorigin: boolean,
* content?: string | undefined,
Expand All @@ -57,23 +57,22 @@ export const generateSRIHash = data => {

/** @type {ElemReplacer} */
const scriptReplacer = (hash, attrs, setCrossorigin, content) =>
`<script${attrs}${hash !== null ? ` integrity="${hash}"` : ''}${
`<script${attrs} integrity="${hash}"${
setCrossorigin ? ' crossorigin="anonymous"' : ''
}>${content ?? ''}</script>`

/** @type {ElemReplacer} */
const styleReplacer = (hash, attrs, setCrossorigin, content) =>
`<style${attrs}${hash !== null ? ` integrity="${hash}"` : ''}${
`<style${attrs} integrity="${hash}"${
setCrossorigin ? ' crossorigin="anonymous"' : ''
}>${content ?? ''}</style>`

/** @type {ElemReplacer} */
const linkStyleReplacer = (hash, attrs, setCrossorigin) =>
`<link${attrs}${hash !== null ? ` integrity="${hash}"` : ''}${
`<link${attrs} integrity="${hash}"${
setCrossorigin ? ' crossorigin="anonymous"' : ''
}/>`

const srcRegex = /\s+(src|href)\s*=\s*("(?<src1>.*?)"|'(?<src2>.*?)')/i
const integrityRegex =
/\s+integrity\s*=\s*("(?<integrity1>.*?)"|'(?<integrity2>.*?)')/i
const relStylesheetRegex = /\s+rel\s*=\s*('stylesheet'|"stylesheet")/i
Expand All @@ -85,6 +84,7 @@ const getRegexProcessors = () => {
t2: 'scripts',
regex:
/<script(?<attrs>(\s+[a-z][a-z0-9\-_]*(=('[^']*?'|"[^"]*?"))?)*?)\s*>(?<content>[\s\S]*?)<\/\s*script\s*>/gi,
srcRegex: /\s+src\s*=\s*("(?<src1>.*?)"|'(?<src2>.*?)')/i,
replacer: scriptReplacer,
hasContent: true,
attrsRegex: undefined,
Expand All @@ -94,6 +94,7 @@ const getRegexProcessors = () => {
t2: 'styles',
regex:
/<style(?<attrs>(\s+[a-z][a-z0-9\-_]*(=('[^']*?'|"[^"]*?"))?)*?)\s*>(?<content>[\s\S]*?)<\/\s*style\s*>/gi,
srcRegex: /\s+(href|src)\s*=\s*("(?<src1>.*?)"|'(?<src2>.*?)')/i, // not really used
replacer: styleReplacer,
hasContent: true,
attrsRegex: undefined,
Expand All @@ -103,6 +104,7 @@ const getRegexProcessors = () => {
t2: 'styles',
regex:
/<link(?<attrs>(\s+[a-z][a-z0-9\-_]*(=('[^']*?'|"[^"]*?"))?)*?)\s*\/?>/gi,
srcRegex: /\s+href\s*=\s*("(?<src1>.*?)"|'(?<src2>.*?)')/i,
replacer: linkStyleReplacer,
hasContent: false,
attrsRegex: relStylesheetRegex,
Expand Down Expand Up @@ -148,11 +150,19 @@ export const updateStaticPageSriHashes = async (
let updatedContent = content
let match

for (const { attrsRegex, hasContent, regex, replacer, t, t2 } of processors) {
for (const {
attrsRegex,
hasContent,
regex,
srcRegex,
replacer,
t,
t2,
} of processors) {
// biome-ignore lint/suspicious/noAssignInExpressions: safe
while ((match = regex.exec(content)) !== null) {
const attrs = match.groups?.attrs ?? ''
const content = match.groups?.content ?? ''
const elemContent = match.groups?.content ?? ''

/** @type {string | undefined} */
let sriHash = undefined
Expand All @@ -167,6 +177,14 @@ export const updateStaticPageSriHashes = async (
const integrityMatch = integrityRegex.exec(attrs)
const src = srcMatch?.groups?.src1 ?? srcMatch?.groups?.src2 ?? ''

if (elemContent && src) {
logger.warn(
`${t} "${src}" must have either a src/href attribute or content, but not both. Removing it.`,
)
updatedContent = updatedContent.replace(match[0], '')
continue
}

if (integrityMatch) {
sriHash =
integrityMatch.groups?.integrity1 ??
Expand Down Expand Up @@ -217,20 +235,22 @@ export const updateStaticPageSriHashes = async (
!(allowInlineScripts === false && t === 'Script') &&
!(allowInlineStyles === false && t === 'Style')
) {
sriHash = generateSRIHash(content)
sriHash = generateSRIHash(elemContent)
h[`inline${t}Hashes`].add(sriHash)
pageHashes[t2].add(sriHash)
} else {
logger.warn(
`Skipping SRI hash generation for inline ${t.toLowerCase()} "${relativeFilepath}" (inline ${t2} are disabled)`,
`Removing inline ${t.toLowerCase()} block (inline ${t2} are disabled).`,
)
updatedContent = updatedContent.replace(match[0], '')
continue
}
}

if (sriHash) {
updatedContent = updatedContent.replace(
match[0],
replacer(sriHash, attrs, setCrossorigin, content),
replacer(sriHash, attrs, setCrossorigin, elemContent),
)
}
}
Expand Down Expand Up @@ -261,17 +281,26 @@ export const updateDynamicPageSriHashes = async (
styles: new Set(),
})

for (const { attrsRegex, hasContent, regex, replacer, t, t2 } of processors) {
for (const {
attrsRegex,
hasContent,
regex,
srcRegex,
replacer,
t,
t2,
} of processors) {
// biome-ignore lint/suspicious/noAssignInExpressions: safe
while ((match = regex.exec(content)) !== null) {
const attrs = match.groups?.attrs ?? ''
const content = match.groups?.content ?? ''
const elemContent = match.groups?.content ?? ''

/** @type {string | undefined} */
let sriHash = undefined
let setCrossorigin = false

if (attrs) {
// This is to skip <link> elements that are not stylesheets
if (attrsRegex && !attrsRegex.test(attrs)) {
continue
}
Expand All @@ -280,33 +309,57 @@ export const updateDynamicPageSriHashes = async (
const integrityMatch = integrityRegex.exec(attrs)
const src = srcMatch?.groups?.src1 ?? srcMatch?.groups?.src2

if (content && src) {
if (elemContent && src) {
logger.warn(
`scripts must have either a src attribute or content, but not both "${src}"`,
`${t} "${src}" must have either a src/href attribute or content, but not both. Removing it.`,
)
updatedContent = updatedContent.replace(match[0], '')
continue
}

if (integrityMatch) {
sriHash =
const givenSriHash =
integrityMatch.groups?.integrity1 ??
integrityMatch.groups?.integrity2
if (sriHash) {
if (givenSriHash) {
if (src) {
const globalHash = globalHashes[t2].get(src)
if (globalHash) {
if (globalHash !== sriHash) {
throw new Error(
`SRI hash mismatch for "${src}", expected "${globalHash}" but got "${sriHash}"`,
if (globalHash !== givenSriHash) {
logger.warn(
`Detected integrity hash mismatch for resource "${src}". Removing it.`,
)
updatedContent = updatedContent.replace(match[0], '')
} else {
sriHash = givenSriHash
pageHashes[t2].add(sriHash)
}
} else {
globalHashes[t2].set(src, sriHash)
logger.warn(
`Detected reference to not explicitly allowed external resource "${src}". Removing it.`,
)
updatedContent = updatedContent.replace(match[0], '')
}
} else if (elemContent) {
if (
(t2 === 'scripts' &&
(sri?.allowInlineScripts ?? 'all') === 'all') ||
(t2 === 'styles' && (sri?.allowInlineStyles ?? 'all') === 'all')
) {
sriHash = givenSriHash
pageHashes[t2].add(sriHash)
} else {
logger.warn(
`Removing inline ${t.toLowerCase()} block (inline ${t2} are disabled).`,
)
updatedContent = updatedContent.replace(match[0], '')
}
}
pageHashes[t2].add(sriHash)
} else {
logger.warn('Found empty integrity attribute, skipping...')
logger.warn(
`Found empty integrity attribute, removing inline ${t.toLowerCase()} block.`,
)
updatedContent = updatedContent.replace(match[0], '')
}
continue
}
Expand All @@ -325,6 +378,7 @@ export const updateDynamicPageSriHashes = async (
src.indexOf('?astro&type=') >= 0
)
) {
// TODO: Perform fetch operation when running in dev mode
logger.warn(
`Unable to obtain SRI hash for local resource: "${src}"`,
)
Expand All @@ -339,49 +393,39 @@ export const updateDynamicPageSriHashes = async (
pageHashes[t2].add(sriHash)
} else {
logger.warn(
`Detected reference to not-allow-listed external resource "${src}"`,
`Detected reference to not explicitly allowed external resource "${src}". Removing it.`,
)
if (setCrossorigin) {
updatedContent = updatedContent.replace(
match[0],
replacer(null, attrs, true, ''),
)
}
updatedContent = updatedContent.replace(match[0], '')
continue

// TODO: add scape hatch to allow fetching arbitrary external resources
// const resourceResponse = await fetch(src, { method: 'GET' })
// const resourceContent = await resourceResponse.arrayBuffer()
// sriHash = generateSRIHash(resourceContent)
// globalHashes[t2].set(src, sriHash)
// pageHashes[t2].add(sriHash)
}
} else {
logger.warn(`Unable to process external resource: "${src}"`)
// TODO: Introduce flag to decide if external resources using unknown protocols should be removed
logger.warn(`Unable to process external resource: "${src}".`)
continue
}
}
}

if (hasContent && !sriHash) {
// TODO: port logic from `updateStaticPageSriHashes` to handle inline resources
if (
((sri?.allowInlineScripts ?? 'all') === 'all' && t === 'Script') ||
((sri?.allowInlineStyles ?? 'all') === 'all' && t === 'Style')
) {
sriHash = generateSRIHash(content)
sriHash = generateSRIHash(elemContent)
pageHashes[t2].add(sriHash)
} else {
logger.warn(
`Skipping SRI hash generation for inline ${t.toLowerCase()} (inline ${t2} are disabled)`,
`Removing inline ${t.toLowerCase()} block (inline ${t2} are disabled)`,
)
updatedContent = updatedContent.replace(match[0], '')
continue
}
}

if (sriHash) {
updatedContent = updatedContent.replace(
match[0],
replacer(sriHash, attrs, setCrossorigin, content),
replacer(sriHash, attrs, setCrossorigin, elemContent),
)
}
}
Expand Down Expand Up @@ -697,7 +741,6 @@ export const processStaticFiles = async (logger, { distDir, sri }) => {
sri,
)


if (!sri.hashesModule) {
return
}
Expand Down
Loading

0 comments on commit 5ae8b8e

Please sign in to comment.