Python: modernise Security/ queries #2739

RasmusWL · 2020-01-31T16:11:14Z

No description provided.

ReflectedXss was the only query that used it with the "a"

Since `IntObjectInternal` extends `TInt`, and `TInt` is defined for all instances of `Builtin.intValue`, and `Builtin.intValue` includes both `int` and `long`, we don't need to handles Longs in a special manner, as we did in NumericObject.

also fixes test code to use the right argument name

Replacing `Value.booleanValue`. We wanted to match `Object.booleanValue` that only gives a result if it is either `true` or `false`, but also wanted to keep the flexibility to see if the Value _could_ be `true`/`false`. We don't have a motivating usecase, so let's see if we ever need it :P + fix modernisation regression on py/jinja2/autoescape-false

RasmusWL · 2020-02-04T10:42:34Z

Ready to go now, except for one last question.

The characteristic predicate for NumericObject is

class NumericObject extends Object {
    NumericObject() {
        this.asBuiltin().getClass() = theIntType().asBuiltin() or
        this.asBuiltin().getClass() = theLongType().asBuiltin() or
        this.asBuiltin().getClass() = theFloatType().asBuiltin()
    }

but for NumericValue (added in this PR), I was not able to find a LongObjectInternal, and was wondering if we needed to add it or not.

class NumericValue extends Value {
    NumericValue() {
        this instanceof IntObjectInternal or
        this instanceof FloatObjectInternal
        // TODO: Should we have a LongObjectInternal ?
    }

Answering my own questions:

I concluded that we don't need to add it, and here is my reasoning.

I searched for uses of theLongType(), and it was only used in 3 places:

In IntegerLiteral.getLiteralObject

https://github.com/Semmle/ql/blob/c525ab325f7a8edbf3eebbfcaaec1e34d052e141/python/ql/src/semmle/python/Exprs.qll#L446-L450

and in NumericObject.intValue()

https://github.com/Semmle/ql/blob/66899942851a850017787f7a04f9b3a2278eb2d7/python/ql/src/semmle/python/types/Object.qll#L244-L252

It is also used in NumericObject.repr(), but I'm ignoring that for now.

_

Since NumericValue includes all IntObjectInternal, and IntObjectInternal extends TInt, and TInt is defined for all instances of Builtin.intValue, we can conclude that with NumericValue we cover all the cases that NumericObject does, and therefore we don't need LongObjectInternal.

https://github.com/Semmle/ql/blob/662aedcb133a47d04a2518f5442b12ecc2d4593b/python/ql/src/semmle/python/types/Builtins.qll#L90-L94

felicitymay · 2020-02-04T10:59:19Z

I think I was automatically added as a reviewer when you added a comma to a qhelp file. Feel free to add me back if you later add changes to this PR that need a docs review 😄

tausbn

A few comments, otherwise LGTM.

python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql

python/ql/src/Security/CWE-022/TarSlip.ql

python/ql/src/Security/CWE-089/SqlInjection.ql

python/ql/src/semmle/python/objects/ObjectAPI.qll

tausbn · 2020-02-17T11:46:59Z

python/ql/test/query-tests/Security/CWE-798/test.py

+test(password='format_string_{}')
+
+# TODO: we think this is a format string :\
+test(password='''U]E8FPETCS_]{,y>bgyzh^$yC5>SP{E*2=`;3]G~k&+;khy3}4]jdpu;D(aP$SCFA{;hh4n46pUJ%+$nEP_gqNq#X!2$%*C-6y6%''')


And this means we do not consider it to be a string? That sounds wrong to me.

No, it's just that if it's a format string, then we don't consider it to be a credentials https://github.com/Semmle/ql/blob/c1d073a54dd299393fecc550b594c4a973278dbf/python/ql/src/Security/CWE-798/HardcodedCredentials.ql#L69

Ah, this makes sense. We wouldn't want to flag random format strings as containing credentials (though I wonder how often this happens. We might want to see what the impact of removing not format_string(str) is on LGTM).

Python: Autoformat security

2648e34

RasmusWL added the Python label Jan 31, 2020

RasmusWL added 3 commits February 3, 2020 14:35

Python: Consistenly use "a user-provided value"

5bc5925

ReflectedXss was the only query that used it with the "a"

Python: Fix minor problems in security examples

27a7d09

Python: Value::forString and friends returns StringValue

d30e6d2

RasmusWL force-pushed the python-modernise-security branch 2 times, most recently from cc2d727 to 94750fc Compare February 4, 2020 10:15

RasmusWL added 7 commits February 4, 2020 11:39

Python: Add NumericValue

2802ac2

Since `IntObjectInternal` extends `TInt`, and `TInt` is defined for all instances of `Builtin.intValue`, and `Builtin.intValue` includes both `int` and `long`, we don't need to handles Longs in a special manner, as we did in NumericObject.

Python: Modernise Security/ queries

e5abfd0

Python: Fix modernisation regression on py/weak-crypto-key

bd1f21f

also fixes test code to use the right argument name

Python: Show how pointsTo handles 0+0 == 0 (1/2)

4231bb1

Python: Show how pointsTo handles 0+0 == 0 (2/2)

2837f98

Python: Add test-cases for py/hardcoded-credentials

c1d073a

RasmusWL force-pushed the python-modernise-security branch from 94750fc to c1d073a Compare February 4, 2020 10:42

RasmusWL changed the title ~~WIP Python: modernise Security/ queries~~ Python: modernise Security/ queries Feb 4, 2020

RasmusWL marked this pull request as ready for review February 4, 2020 10:42

RasmusWL requested review from felicitymay and a team as code owners February 4, 2020 10:42

felicitymay removed their request for review February 4, 2020 10:58

tausbn self-assigned this Feb 12, 2020

RasmusWL mentioned this pull request Feb 14, 2020

Python: ObjectAPI to ValueAPI: TruncatedDivision #2817

Merged

tausbn requested changes Feb 17, 2020

View reviewed changes

RasmusWL added 2 commits February 17, 2020 14:34

Python: Fix typos

6d5a8e4

Python: Use StringValue instead of Value::forString

f3ab52b

tausbn approved these changes Feb 18, 2020

View reviewed changes

tausbn merged commit ffbb5d0 into github:master Feb 18, 2020

RasmusWL deleted the python-modernise-security branch February 18, 2020 15:39

kamarcum unassigned tausbn Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Python: modernise Security/ queries #2739

Python: modernise Security/ queries #2739

Uh oh!

RasmusWL commented Jan 31, 2020

Uh oh!

RasmusWL commented Feb 4, 2020

Uh oh!

felicitymay commented Feb 4, 2020

Uh oh!

tausbn left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tausbn Feb 17, 2020

Uh oh!

RasmusWL Feb 17, 2020

Uh oh!

tausbn Feb 17, 2020

Uh oh!

Uh oh!

Python: modernise Security/ queries #2739

Python: modernise Security/ queries #2739

Uh oh!

Conversation

RasmusWL commented Jan 31, 2020

Uh oh!

RasmusWL commented Feb 4, 2020

Answering my own questions:

_

Uh oh!

felicitymay commented Feb 4, 2020

Uh oh!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tausbn Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

RasmusWL Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

tausbn Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!