From 2ce1192ee6e5f333b5eaf03ba69297e4a5e27666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 11 Jul 2023 10:28:14 +0200 Subject: [PATCH 1/6] implement field taint inheritance for Struts2 unmarshalled objects --- .../lib/semmle/code/java/Serializability.qll | 1 + .../struts/Struts2Serializability.qll | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll diff --git a/java/ql/lib/semmle/code/java/Serializability.qll b/java/ql/lib/semmle/code/java/Serializability.qll index 724901180208..f665f663c7e2 100644 --- a/java/ql/lib/semmle/code/java/Serializability.qll +++ b/java/ql/lib/semmle/code/java/Serializability.qll @@ -6,6 +6,7 @@ import java private import frameworks.jackson.JacksonSerializability private import frameworks.google.GsonSerializability private import frameworks.google.GoogleHttpClientApi +private import frameworks.struts.Struts2Serializability /** * A serializable field may be read without code referencing it, diff --git a/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll b/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll new file mode 100644 index 000000000000..8768b61cec2e --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll @@ -0,0 +1,47 @@ +/** + * Provides classes and predicates for working with objects bound from Http requests in the context of + * the Struts2 web framework. + */ + +import java +private import semmle.code.java.Serializability +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.FlowSteps +private import semmle.code.java.frameworks.struts.StrutsActions + +/** A type whose values may be unmarshalled from an Http request by the Struts2 framework. */ +abstract class Struts2DeserializableType extends Type { } + +/** A type whose values are explicitly unmarshalled by from an Http request by the Struts2 framework. */ +private class ExplicitlyReadStruts2DeserializableType extends Struts2DeserializableType { + ExplicitlyReadStruts2DeserializableType() { + exists(Struts2ActionSupportClass c | + usesType(c.getASetterMethod().getField().getType(), this) and + not this instanceof TypeClass and + not this instanceof TypeObject + ) + } +} + +/** A type used in a `Struts2ActionField` declaration. */ +private class FieldReferencedStruts2DeserializableType extends Struts2DeserializableType { + FieldReferencedStruts2DeserializableType() { + exists(Struts2ActionField f | usesType(f.getType(), this)) + } +} + +/** A field that may be unmarshalled from an Http request using the Struts2 framework. */ +private class Struts2ActionField extends DeserializableField { + Struts2ActionField() { + exists(Struts2DeserializableType superType | + superType = this.getDeclaringType().getAnAncestor() and + not superType instanceof TypeObject and + superType.fromSource() + ) + } +} + +/** A field that should convey the taint from its qualifier to itself. */ +private class Struts2ActionFieldInheritTaint extends DataFlow::FieldContent, TaintInheritingContent { + Struts2ActionFieldInheritTaint() { this.getField() instanceof Struts2ActionField } +} From ff1ae7d9c629e15dbdf5e7652ac6c177da32b2fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Wed, 12 Jul 2023 11:05:25 +0200 Subject: [PATCH 2/6] add change note --- .../2023-07-12-improve-sources-for-the-struts2-framework.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md diff --git a/java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md b/java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md new file mode 100644 index 000000000000..6e81383751f5 --- /dev/null +++ b/java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* Improved the modeling of Struts 2 sources of untrusted data by tainting the whole object graph of the objects unmarshaled from an HTTP request. + From f3fc56294e25bd584c64053aa7e81b246648abbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 11 Jul 2023 10:28:14 +0200 Subject: [PATCH 3/6] implement field taint inheritance for Struts2 unmarshalled objects --- .../lib/semmle/code/java/Serializability.qll | 1 + .../struts/Struts2Serializability.qll | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll diff --git a/java/ql/lib/semmle/code/java/Serializability.qll b/java/ql/lib/semmle/code/java/Serializability.qll index 724901180208..f665f663c7e2 100644 --- a/java/ql/lib/semmle/code/java/Serializability.qll +++ b/java/ql/lib/semmle/code/java/Serializability.qll @@ -6,6 +6,7 @@ import java private import frameworks.jackson.JacksonSerializability private import frameworks.google.GsonSerializability private import frameworks.google.GoogleHttpClientApi +private import frameworks.struts.Struts2Serializability /** * A serializable field may be read without code referencing it, diff --git a/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll b/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll new file mode 100644 index 000000000000..8768b61cec2e --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll @@ -0,0 +1,47 @@ +/** + * Provides classes and predicates for working with objects bound from Http requests in the context of + * the Struts2 web framework. + */ + +import java +private import semmle.code.java.Serializability +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.FlowSteps +private import semmle.code.java.frameworks.struts.StrutsActions + +/** A type whose values may be unmarshalled from an Http request by the Struts2 framework. */ +abstract class Struts2DeserializableType extends Type { } + +/** A type whose values are explicitly unmarshalled by from an Http request by the Struts2 framework. */ +private class ExplicitlyReadStruts2DeserializableType extends Struts2DeserializableType { + ExplicitlyReadStruts2DeserializableType() { + exists(Struts2ActionSupportClass c | + usesType(c.getASetterMethod().getField().getType(), this) and + not this instanceof TypeClass and + not this instanceof TypeObject + ) + } +} + +/** A type used in a `Struts2ActionField` declaration. */ +private class FieldReferencedStruts2DeserializableType extends Struts2DeserializableType { + FieldReferencedStruts2DeserializableType() { + exists(Struts2ActionField f | usesType(f.getType(), this)) + } +} + +/** A field that may be unmarshalled from an Http request using the Struts2 framework. */ +private class Struts2ActionField extends DeserializableField { + Struts2ActionField() { + exists(Struts2DeserializableType superType | + superType = this.getDeclaringType().getAnAncestor() and + not superType instanceof TypeObject and + superType.fromSource() + ) + } +} + +/** A field that should convey the taint from its qualifier to itself. */ +private class Struts2ActionFieldInheritTaint extends DataFlow::FieldContent, TaintInheritingContent { + Struts2ActionFieldInheritTaint() { this.getField() instanceof Struts2ActionField } +} From 97a4230d5d7bd6cda04ec900c05dbcdc9bf0fff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Wed, 12 Jul 2023 11:05:25 +0200 Subject: [PATCH 4/6] add change note --- .../2023-07-12-improve-sources-for-the-struts2-framework.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md diff --git a/java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md b/java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md new file mode 100644 index 000000000000..6e81383751f5 --- /dev/null +++ b/java/ql/lib/change-notes/2023-07-12-improve-sources-for-the-struts2-framework.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* Improved the modeling of Struts 2 sources of untrusted data by tainting the whole object graph of the objects unmarshaled from an HTTP request. + From c239a4399cc5a86c4638737a728d069034cb4845 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 27 Jul 2023 10:37:00 +0200 Subject: [PATCH 5/6] Changed Struts2ActionSupportClassFieldReadSource to be a FieldValueNode instead of a field read --- java/ql/lib/semmle/code/java/dataflow/FlowSources.qll | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll b/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll index f049a0cb37be..26f29076efc4 100644 --- a/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll @@ -143,11 +143,10 @@ private class GuiceRequestParameterSource extends RemoteFlowSource { override string getSourceType() { result = "Guice request parameter" } } -private class Struts2ActionSupportClassFieldReadSource extends RemoteFlowSource { - Struts2ActionSupportClassFieldReadSource() { - exists(Struts2ActionSupportClass c | - c.getASetterMethod().getField() = this.asExpr().(FieldRead).getField() - ) +private class Struts2ActionSupportClassFieldSource extends RemoteFlowSource { + Struts2ActionSupportClassFieldSource() { + this.(DataFlow::FieldValueNode).getField() = + any(Struts2ActionSupportClass c).getASetterMethod().getField() } override string getSourceType() { result = "Struts2 ActionSupport field" } From c3a2ae2943d6aed65cb260a3c7e8e5140e88e9ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 28 Jul 2023 12:12:07 +0200 Subject: [PATCH 6/6] Account for public fields/setters --- .../code/java/frameworks/struts/Struts2Serializability.qll | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll b/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll index 8768b61cec2e..cb8b876be7ad 100644 --- a/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll +++ b/java/ql/lib/semmle/code/java/frameworks/struts/Struts2Serializability.qll @@ -36,7 +36,12 @@ private class Struts2ActionField extends DeserializableField { exists(Struts2DeserializableType superType | superType = this.getDeclaringType().getAnAncestor() and not superType instanceof TypeObject and - superType.fromSource() + superType.fromSource() and + ( + this.isPublic() + or + exists(SetterMethod setter | setter.getField() = this and setter.isPublic()) + ) ) } }