deserialization sinks · Pull Request #5508 · github/codeql

ghost · 2021-03-23T21:25:53Z

Added twenty new sinks and verification if user controls instantiated type in strong type serializer case.

tamasvajk

Thank you for this contribution. This PR is rather large and therefore a bit difficult to review. I didn't yet check all the deserializer callables, weak/strong type deserializers, but I wanted to give you an initial review.

ghost · 2021-04-09T09:30:05Z

Ping

tamasvajk · 2021-04-09T11:18:10Z

Ping

Sorry for the delay, I'm a bit behind on code reviews. I'll catch up until mid next week. If not, feel free to ping me again.

hvitved

Some initial comments. Thanks for you contributions.

hvitved · 2021-04-22T07:41:52Z

The QLDoc check fails with

Error: The following CodeQL elements are lacking documentation:
semmle/code/csharp/security/dataflow/UnsafeDeserialization.qll  UnsafeDeserialization::UnsafeDeserialization::LocalSource                 class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ActivityLoadMethod                                         class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::BinaryFormatterDeserializeMethod                           class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::BinaryFormatterUnsafeDeserializeMethod                     class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::BinaryFormatterUnsafeDeserializeMethodResponseMethod       class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::BinaryMessageFormatterReadMethod                           class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::DataContractJsonSerializerReadObjectMethod                 class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::DataContractSerializerReadObjectMethod                     class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::FastJsonClassToObjectMethod                                class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::JavaScriptSerializerClassDeserializeMethod                 class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::JavaScriptSerializerClassDeserializeObjectMethod           class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::JaysonConverterToObjectMethod                              class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::LosFormatterDeserializeMethod                              class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::NetDataContractSerializerDeserializeMethod                 class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::NetDataContractSerializerReadObjectMethod                  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ObjectStateFormatterDeserializeMethod                      class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ProxyObjectDecodeSerializedObjectMethod                    class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ProxyObjectDecodeValueMethod                               class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ResourceReaderConstructor                                  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextCsvSerializerDeserializeFromReaderMethod   class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextCsvSerializerDeserializeFromStreamMethod   class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextCsvSerializerDeserializeFromStringMethod   class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextJsonSerializerDeserializeFromReaderMethod  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextJsonSerializerDeserializeFromStreamMethod  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextJsonSerializerDeserializeFromStringMethod  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextTypeSerializerDeserializeFromReaderMethod  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextTypeSerializerDeserializeFromStreamMethod  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextTypeSerializerDeserializeFromStringMethod  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextXmlSerializerDeserializeFromReaderMethod   class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextXmlSerializerDeserializeFromStreamMethod   class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::ServiceStackTextXmlSerializerDeserializeFromStringMethod   class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::SoapFormatterDeserializeMethod                             class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::XamlReaderLoadAsyncMethod                                  class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::XamlReaderLoadMethod                                       class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::XamlReaderParseMethod                                      class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::XmlMessageFormatterReadMethod                              class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::XmlObjectSerializerReadObjectMethod                        class
semmle/code/csharp/serialization/Deserializers.qll              Deserializers::XmlSerializerDeserializeMethod                             class

So either add doc, or make the classes private.

ghost · 2021-04-22T09:04:12Z

So either add doc, or make the classes private.

If I make for example BinaryFormatterDeserializeMethod private it breaks with ERROR: Could not resolve type BinaryFormatterDeserializeMethod (/csharp/ql/src/semmle/code/csharp/security/dataflow/UnsafeDeserialization.qll:152... which is because of

  /** BinaryFormatter */
  private predicate isBinaryFormatterCall(MethodCall mc, Method m) {
    m = mc.getTarget() and
    (
      not mc.getArgument(0).hasValue() and
      (
        m instanceof BinaryFormatterDeserializeMethod
        or
        m instanceof BinaryFormatterUnsafeDeserializeMethod
        or
        m instanceof BinaryFormatterUnsafeDeserializeMethodResponseMethod
      )
    )
  }

Should I merge UnsafeDeserialization.qll and Deserializers.qll into one?

hvitved · 2021-04-22T09:54:13Z

Should I merge UnsafeDeserialization.qll and Deserializers.qll into one?

Then I think it is better to add documentation for the classes.

hvitved · 2021-05-05T15:26:38Z

Here is a run of the two queries comparing results before and after:

UnsafeDeserializationUntrustedInput.ql: https://lgtm.com/query/5618697988968865317/
UnsafeDeserialization.ql: https://lgtm.com/query/4515885059546123954/

ghost · 2021-05-10T09:23:34Z

Here is a run of the two queries comparing results before and after:

* `UnsafeDeserializationUntrustedInput.ql`: https://lgtm.com/query/5618697988968865317/

* `UnsafeDeserialization.ql`: https://lgtm.com/query/4515885059546123954/

Probably the query is more valuable for corporate clients than for open source.

pwntester · 2021-05-13T10:15:53Z

+    this instanceof ServiceStackTextCsvSerializerClass
+    or
+    this instanceof ServiceStackTextXmlSerializerClass
  }


It would be nice to complete these lists with:

YamlDotNet

FsPickler

Json.Net

SharpSerializerBinary

SharpSerializerXml

And maybe LitJSON according to this tweet: https://twitter.com/frycos/status/1391497640425279489

I have added support for YamlDotNet (pre 5.x version), FsPickler, Json.Net and SharpSerializer.
I didn't add LitJson because in case of ToObject<T> (string json) the T cannot be user controlled. This leaves only ToObject(string json, Type ConvertType ), but my search didn't return any usages.

Please review.

ghost · 2021-07-26T20:07:03Z

Ping

hvitved · 2021-08-02T15:35:27Z

Please resolve merge conflicts.

hvitved · 2021-08-09T07:45:21Z

I will perform an internal test for performance regressions; I'll let you know once it's done.

hvitved · 2021-08-17T09:40:33Z

As expected, UnsafeDeserializationUntrustedInput.ql is now slower than before (e.g., it now takes ~600s instead of ~100s on mono/mono). However, I was unable to fix this, and appears to be simply because we are now computing more data flow.

Comments have been addressed.

deserialization sinks

ac29184

ghost self-requested a review as a code owner March 23, 2021 21:25

github-actions Bot added C# documentation labels Mar 23, 2021

tamasvajk self-assigned this Mar 25, 2021

tamasvajk suggested changes Mar 29, 2021

View reviewed changes

edvraa added 6 commits March 31, 2021 15:22

Rename ObjectMethodSink to InstanceMethodSink

94234b8

Fix comment

8bb3be2

Simplify query

7cbbd6c

Rename taint tracking variables

aa9d848

Rename deserializeCall to deserializeCallArg

f8867e4

Make query symmetric

1308070

ghost mentioned this pull request Apr 9, 2021

[C#]: Deserialization sinks github/securitylab#337

Closed

1 task

tamasvajk previously requested changes Apr 12, 2021

View reviewed changes

Comment thread csharp/ql/src/Security Features/CWE-502/UnsafeDeserialization.ql Outdated

Comment thread csharp/ql/src/Security Features/CWE-502/UnsafeDeserialization.ql Outdated

Comment thread csharp/ql/src/semmle/code/csharp/security/dataflow/UnsafeDeserialization.qll Outdated

hvitved requested changes Apr 12, 2021

View reviewed changes

edvraa added 7 commits April 14, 2021 23:35

Use don't care expression

a4fd70a

Hide implementation details

3a9d1f4

Remove redundant check

b027fdd

Simplify getTarget check

1581a27

Use hasFlow where path is not needed

773556e

Use TaintTracking2

3aedd2c

reintroduce UnsafeDeserializer

a412581

ghost requested review from hvitved and tamasvajk April 15, 2021 19:33

Charpred for InstanceMethodSink

c3deb48

hvitved requested changes Apr 20, 2021

View reviewed changes

edvraa added 2 commits April 21, 2021 13:27

Move RemoteSource and LocalSource to UnsafeDeserialization.qll

3ac5f7b

a deserializer

0590522

ghost requested a review from hvitved April 21, 2021 14:26

Remove DataFlow::Node

57689df

hvitved reviewed Apr 22, 2021

View reviewed changes

Comment thread ...tures/CWE-502/UnsafeDeserializationUntrustedInput/UnsafeDeserializationUntrustedInputGood.cs Outdated

Make similarly named files in tests and qhelp in sync

c9c9758

add comments

18a3e4d

pwntester reviewed May 13, 2021

View reviewed changes

edvraa and others added 5 commits July 12, 2021 11:32

Merge with Main

1682e99

FsPickler

c3ac3ca

SharpSerializer

1e4409f

YamlDotNet

f4cb6c5

JsonConvert

a0942e0

ghost requested a review from hvitved July 12, 2021 12:41

hvitved requested changes Jul 14, 2021

View reviewed changes

Comment thread csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInput.ql

Comment thread csharp/ql/src/semmle/code/csharp/security/dataflow/UnsafeDeserialization.qll Outdated

Comment thread csharp/ql/src/semmle/code/csharp/security/dataflow/UnsafeDeserialization.qll Outdated

Use HasFlow instead HasFlowPath

fd4d8e2

ghost requested a review from hvitved July 14, 2021 13:08

edvraa added 2 commits August 4, 2021 14:25

Merge with main

d1e4168

Post merge

db2f9ad

hvitved added the no-change-note-required This PR does not need a change note label Aug 5, 2021

hvitved approved these changes Aug 17, 2021

View reviewed changes

hvitved merged commit 44ff623 into github:main Aug 17, 2021

Conversation

ghost commented Mar 23, 2021

Uh oh!

tamasvajk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Apr 9, 2021

Uh oh!

tamasvajk commented Apr 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hvitved commented Apr 22, 2021

Uh oh!

Uh oh!

ghost commented Apr 22, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvitved commented Apr 22, 2021

Uh oh!

hvitved commented May 5, 2021

Uh oh!

ghost commented May 10, 2021

Uh oh!

pwntester May 13, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Jul 26, 2021

Uh oh!

hvitved commented Aug 2, 2021

Uh oh!

hvitved commented Aug 9, 2021

Uh oh!

hvitved commented Aug 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ghost commented Apr 22, 2021 •

edited by ghost

Loading