From 76f2748cbc4bb99d0d09dc2c5a8d39ef70618a71 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 3 Apr 2020 18:05:17 +0100 Subject: [PATCH] Teach `SsaWithFields` to properly handle implicit dereferences. --- change-notes/1.24/analysis-go.md | 1 + ql/src/semmle/go/controlflow/IR.qll | 8 +++++++ ql/src/semmle/go/dataflow/SSA.qll | 10 +++++--- .../Security/CWE-022/ZipSlip.expected | 6 +++-- .../query-tests/Security/CWE-022/tarslip.go | 24 +++++++++++++++++++ 5 files changed, 44 insertions(+), 5 deletions(-) create mode 100644 ql/test/query-tests/Security/CWE-022/tarslip.go diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index f71ef2a1613..fb94cfe9950 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -26,6 +26,7 @@ The CodeQL library for Go now contains a folder of simple "cookbook" queries tha | **Query** | **Expected impact** | **Change** | |-------------------------------------------------------------------------------|-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| Arbitrary file write during zip extraction ("zip slip") (`go/zipslip`) | Fewer false positives | The query now recognizes more cases where it is safe to use a path extractor from an archive. | | Bitwise exclusive-or used like exponentiation (`go/mistyped-exponentiation`) | Fewer false positives | The query now identifies when the value of an xor is assigned to a mask object, and excludes such results. | | Command built from user-controlled sources (`go/command-injection`) | More results | The library models used by the query have been improved, allowing it to flag more potentially problematic cases, including sources that flow into shells, sudo, or programming-language interpreters as arguments. | | Database query built from user-controlled sources (`go/sql-injection`) | More results | The library models used by the query have been improved, allowing it to flag more potentially problematic cases. | diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index 6c0f4d43bdf..82872132c69 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -1555,4 +1555,12 @@ module IR { EvalImplicitMaxSliceBoundInstruction implicitMaxSliceBoundInstruction(SliceExpr e) { result = MkImplicitMaxSliceBound(e) } + + /** + * Gets the implicit dereference instruction for `e`, where `e` is a pointer used as the base + * in a field/method access, element access, or slice expression. + */ + EvalImplicitDerefInstruction implicitDerefInstruction(Expr e) { + result = MkImplicitDeref(e) + } } diff --git a/ql/src/semmle/go/dataflow/SSA.qll b/ql/src/semmle/go/dataflow/SSA.qll index 0d05a6aa824..6d92b1c926d 100644 --- a/ql/src/semmle/go/dataflow/SSA.qll +++ b/ql/src/semmle/go/dataflow/SSA.qll @@ -311,9 +311,13 @@ private TSsaWithFields accessPath(IR::Instruction insn) { * by ssa-with-fields value `base`. */ private IR::Instruction accessPathAux(TSsaWithFields base, Field f) { - exists(IR::FieldReadInstruction fr | fr = result | - base = accessPath(fr.getBase()) and - f = fr.getField() + exists(IR::FieldReadInstruction fr, IR::EvalInstruction frb | + fr.getBase() = frb or + fr.getBase() = IR::implicitDerefInstruction(frb.getExpr()) + | + base = accessPath(frb) and + f = fr.getField() and + result = fr ) } diff --git a/ql/test/query-tests/Security/CWE-022/ZipSlip.expected b/ql/test/query-tests/Security/CWE-022/ZipSlip.expected index a58613d40ee..212eb53924d 100644 --- a/ql/test/query-tests/Security/CWE-022/ZipSlip.expected +++ b/ql/test/query-tests/Security/CWE-022/ZipSlip.expected @@ -1,13 +1,15 @@ edges | ZipSlip.go:12:24:12:29 | selection of Name : string | ZipSlip.go:14:20:14:20 | p | +| tarslip.go:14:23:14:33 | selection of Name : string | tarslip.go:14:14:14:34 | call to Dir | | tst.go:15:11:15:16 | selection of Name : string | tst.go:20:20:20:23 | path | nodes | ZipSlip.go:12:24:12:29 | selection of Name : string | semmle.label | selection of Name : string | | ZipSlip.go:14:20:14:20 | p | semmle.label | p | +| tarslip.go:14:14:14:34 | call to Dir | semmle.label | call to Dir | +| tarslip.go:14:23:14:33 | selection of Name : string | semmle.label | selection of Name : string | | tst.go:15:11:15:16 | selection of Name : string | semmle.label | selection of Name : string | | tst.go:20:20:20:23 | path | semmle.label | path | -| tst.go:32:21:32:26 | selection of Name | semmle.label | selection of Name | #select | ZipSlip.go:12:24:12:29 | selection of Name | ZipSlip.go:12:24:12:29 | selection of Name : string | ZipSlip.go:14:20:14:20 | p | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.go:14:20:14:20 | p | file system operation | +| tarslip.go:14:23:14:33 | selection of Name | tarslip.go:14:23:14:33 | selection of Name : string | tarslip.go:14:14:14:34 | call to Dir | Unsanitized archive entry, which may contain '..', is used in a $@. | tarslip.go:14:14:14:34 | call to Dir | file system operation | | tst.go:15:11:15:16 | selection of Name | tst.go:15:11:15:16 | selection of Name : string | tst.go:20:20:20:23 | path | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:20:20:20:23 | path | file system operation | -| tst.go:32:21:32:26 | selection of Name | tst.go:32:21:32:26 | selection of Name | tst.go:32:21:32:26 | selection of Name | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:32:21:32:26 | selection of Name | file system operation | diff --git a/ql/test/query-tests/Security/CWE-022/tarslip.go b/ql/test/query-tests/Security/CWE-022/tarslip.go new file mode 100644 index 00000000000..b81c8d30b94 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-022/tarslip.go @@ -0,0 +1,24 @@ +package main + +import ( + "archive/tar" + "io" + "os" + "path" + "strings" +) + +func untarBad(reader io.Reader, prefix string) { + tarReader := tar.NewReader(reader) + header, _ := tarReader.Next() + os.MkdirAll(path.Dir(header.Name), 0755) // NOT OK +} + +func untarGood(reader io.Reader, prefix string) { + tarReader := tar.NewReader(reader) + header, _ := tarReader.Next() + if !strings.HasPrefix(header.Name, prefix) { + panic("tar contents corrupted") + } + os.MkdirAll(path.Dir(header.Name), 0755) // OK +}