-
Notifications
You must be signed in to change notification settings - Fork 173
[Aikido] AI Fix for Potential file inclusion attack via reading file #22
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import * as accuracy from '../lib/accuracy' | |
| const challengeUtils = require('../lib/challengeUtils') | ||
| const fs = require('fs') | ||
| const yaml = require('js-yaml') | ||
| const path = require('path') | ||
|
|
||
| const FixesDir = 'data/static/codefixes' | ||
|
|
||
|
|
@@ -76,8 +77,11 @@ export const checkCorrectFix = () => async (req: Request<Record<string, unknown> | |
| }) | ||
| } else { | ||
| let explanation | ||
| if (fs.existsSync('./data/static/codefixes/' + key + '.info.yml')) { | ||
| const codingChallengeInfos = yaml.load(fs.readFileSync('./data/static/codefixes/' + key + '.info.yml', 'utf8')) | ||
| const base = path.resolve('./data/static/codefixes') | ||
| const target = path.resolve(base, key + '.info.yml') | ||
| const relative = path.relative(base, target) | ||
| if (!relative.startsWith('..') && !path.isAbsolute(relative) && fs.existsSync(target)) { | ||
| const codingChallengeInfos = yaml.load(fs.readFileSync(target, 'utf8')) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe yaml load can lead to remote code execution - medium severity Show fixRemediation: Ignore this issue only if you will always load YAML documents from trusted origins. If possible, use JSON. Reply |
||
| const selectedFixInfo = codingChallengeInfos?.fixes.find(({ id }: { id: number }) => id === selectedFix + 1) | ||
| if (selectedFixInfo?.explanation) explanation = res.__(selectedFixInfo.explanation) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,8 +90,11 @@ exports.checkVulnLines = () => async (req: Request<Record<string, unknown>, Reco | |
| const selectedLines: number[] = req.body.selectedLines | ||
| const verdict = getVerdict(vulnLines, neutralLines, selectedLines) | ||
| let hint | ||
| if (fs.existsSync('./data/static/codefixes/' + key + '.info.yml')) { | ||
| const codingChallengeInfos = yaml.load(fs.readFileSync('./data/static/codefixes/' + key + '.info.yml', 'utf8')) | ||
| const base = path.resolve('./data/static/codefixes') | ||
| const target = path.resolve(base, key + '.info.yml') | ||
| const relative = path.relative(base, target) | ||
| if (!relative.startsWith('..') && !path.isAbsolute(relative) && fs.existsSync(target)) { | ||
| const codingChallengeInfos = yaml.load(fs.readFileSync(target, 'utf8')) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsafe yaml load can lead to remote code execution - medium severity Show fixRemediation: Ignore this issue only if you will always load YAML documents from trusted origins. If possible, use JSON. Reply |
||
| if (codingChallengeInfos?.hints) { | ||
| if (accuracy.getFindItAttempts(key) > codingChallengeInfos.hints.length) { | ||
| if (vulnLines.length === 1) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,13 @@ const checkDiffs = async (keys: string[]) => { | |
| .then(snippet => { | ||
| if (snippet == null) return | ||
| process.stdout.write(val + ': ') | ||
| const fileData = fs.readFileSync(fixesPath + '/' + val).toString() | ||
| const base = path.resolve(fixesPath) | ||
| const target = path.resolve(base, val) | ||
| const relative = path.relative(base, target) | ||
| if (relative.startsWith('..') || path.isAbsolute(relative)) { | ||
| throw new Error('Invalid file path') | ||
| } | ||
| const fileData = fs.readFileSync(target).toString() | ||
| const diff = Diff.diffLines(filterString(fileData), filterString(snippet.snippet)) | ||
| let line = 0 | ||
| for (const part of diff) { | ||
|
|
@@ -100,7 +106,13 @@ const checkDiffs = async (keys: string[]) => { | |
| } | ||
|
|
||
| async function seePatch (file: string) { | ||
| const fileData = fs.readFileSync(fixesPath + '/' + file).toString() | ||
| const resolvedBase = path.resolve(fixesPath) | ||
| const resolvedTarget = path.resolve(resolvedBase, file) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential file inclusion attack via reading file - medium severity Show fixRemediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend! Reply |
||
| const relativePath = path.relative(resolvedBase, resolvedTarget) | ||
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | ||
| throw new Error('Invalid file path') | ||
| } | ||
| const fileData = fs.readFileSync(resolvedTarget).toString() | ||
| const snippet = await retrieveCodeSnippet(file.split('_')[0]) | ||
| if (snippet == null) return | ||
| const patch = Diff.structuredPatch(file, file, filterString(snippet.snippet), filterString(fileData)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply
@AikidoSec ignore: [REASON]to ignore this issue.More info