[JAVA] pre-defined query fails to detect CWE-502.

[jiezhuzzz]

I'm running pre-defined query to detect a identified CWE-502 vulnerability jfinal/jfinal#184. The root cause of this issue is clear, as it use the readObject function without any extra operations. But I got a blank result after I ran the pre-defined query. I took a look at the implementation, and I tried this predicate which worked well.

codeql/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll

Lines 148 to 235 in 5b1cae5

	predicate unsafeDeserialization(MethodCall ma, Expr sink) {
	exists(Method m | m = ma.getMethod() |
	m instanceof ObjectInputStreamReadObjectMethod and
	sink = ma.getQualifier() and
	not exists(DataFlow::ExprNode node |
	node.getExpr() = sink and
	node.getTypeBound() instanceof SafeObjectInputStreamType
	)
	or
	m instanceof XmlDecoderReadObjectMethod and
	sink = ma.getQualifier()
	or
	m instanceof XStreamReadObjectMethod and
	sink = ma.getAnArgument() and
	not SafeXStreamFlow::flowToExpr(ma.getQualifier())
	or
	m instanceof KryoReadObjectMethod and
	sink = ma.getAnArgument() and
	not SafeKryoFlow::flowToExpr(ma.getQualifier())
	or
	m instanceof MethodApacheSerializationUtilsDeserialize and
	sink = ma.getArgument(0)
	or
	ma instanceof UnsafeSnakeYamlParse and
	sink = ma.getArgument(0)
	or
	ma.getMethod() instanceof FastJsonParseMethod and
	not fastJsonLooksSafe() and
	sink = ma.getArgument(0)
	or
	ma.getMethod() instanceof JYamlLoaderUnsafeLoadMethod and
	sink = ma.getArgument(0)
	or
	ma.getMethod() instanceof JsonIoJsonToJavaMethod and
	sink = ma.getArgument(0)
	or
	ma.getMethod() instanceof JsonIoReadObjectMethod and
	sink = ma.getQualifier()
	or
	ma.getMethod() instanceof YamlBeansReaderReadMethod and sink = ma.getQualifier()
	or
	ma.getMethod() instanceof UnsafeHessianInputReadObjectMethod and sink = ma.getQualifier()
	or
	ma.getMethod() instanceof CastorUnmarshalMethod and sink = ma.getAnArgument()
	or
	ma.getMethod() instanceof BurlapInputReadObjectMethod and sink = ma.getQualifier()
	or
	ma.getMethod() instanceof ObjectMapperReadMethod and
	sink = ma.getArgument(0) and
	(
	UnsafeTypeFlow::flowToExpr(ma.getAnArgument())
	or
	EnableJacksonDefaultTypingFlow::flowToExpr(ma.getQualifier())
	or
	hasArgumentWithUnsafeJacksonAnnotation(ma)
	) and
	not SafeObjectMapperFlow::flowToExpr(ma.getQualifier())
	or
	m instanceof JabsorbUnmarshallMethod and
	sink = ma.getArgument(2) and
	UnsafeTypeFlow::flowToExpr(ma.getArgument(1))
	or
	m instanceof JabsorbFromJsonMethod and
	sink = ma.getArgument(0)
	or
	m instanceof JoddJsonParseMethod and
	sink = ma.getArgument(0) and
	(
	// User controls the target type for deserialization
	UnsafeTypeFlow::flowToExpr(ma.getArgument(1))
	or
	// jodd.json.JsonParser may be configured for unrestricted deserialization to user-specified types
	joddJsonParserConfiguredUnsafely(ma.getQualifier())
	)
	or
	m instanceof FlexjsonDeserializeMethod and
	sink = ma.getArgument(0)
	or
	m instanceof GsonDeserializeMethod and
	sink = ma.getArgument(0) and
	UnsafeTypeFlow::flowToExpr(ma.getArgument(1))
	or
	m.getASourceOverriddenMethod*() instanceof ObjectMessageGetObjectMethod and
	sink = ma.getQualifier().getUnderlyingExpr() and
	// If we can see an implementation, we trust dataflow to find a path to the other sinks instead
	not exists(viableCallable(ma))
	)
	}

Any hints about why the pre-defined query cannot detect this issue? Thanks a lot!

[smowton]

I think the answer is that the unsafe-deserialization query finds that vulnerable use-site alright, but all the relevant sources of untrusted information within JFinal are in the core module (e.g., reading HTTP header parameters), while the unsafe code could only be reached via Redis.get and similar functions, but JFinal doesn't itself call those (presumably, user programs built on JFinal would join the two things together).

Another way to say that is, we don't know from the code of JFinal alone what your threat model is: is the stuff stored in Redis safe (you sanitized it on its way into the database), in which case it's fine to deserialize it to an arbitrary object, or is it potentially dangerous?

In general something like this would be handled by customising the query to regard jedis.get as a source of dangerous (untrusted) data, and so instructing CodeQL to check for that data flowing to a dangerous call such as readObject.

[jiezhuzzz]

Hi Chris, thanks for your reply! If I understand correctly, CodeQL cannot detect if a source in project A and a sink in project B, because 1. it does not constitute a complete dataflow 2. it is unknown whether sanitization is performed in the middle.

And I found the root cause can be detected as UnsafeDeserializationSink, but since the source is not in the JFinal. We cannot detect the whole dataflow.

Thanks again for your help!