[Java] I've written a rule, but it's a bit flawed and doesn't fully check out the entire path of request->bytes.

[SummerSec]

Now the following code, known index function first parameter request is user input can be used as a source, and then now known to deserialize function points in Tools#deserialize function, can be used as a sink. how to write QL rules to query this vulnerability?

Code：

@GetMapping({"/index/{name}"})
public String index(HttpServletRequest request, HttpServletResponse response, @PathVariable String name) throws Exception {
        Cookie[] cookies = request.getCookies();
        boolean exist = false;
        Cookie cookie = null;
        User user = null;
        if (cookies != null) {
            Cookie[] var8 = cookies;
            int var9 = cookies.length;

            for(int var10 = 0; var10 < var9; ++var10) {
                Cookie c = var8[var10];
                if (c.getName().equals("hacker")) {
                    exist = true;
                    cookie = c;
                    break;
                }
            }
        }

        if (exist) {

            byte[] bytes = Tools.base64Decode(cookie.getValue());
            user = (User)Tools.deserialize(bytes);


        } else {
            user = new User();
            user.setID(1);
            user.setUserName(name);
            cookie = new Cookie("hacker", Tools.base64Encode(Tools.serialize(user)));
            response.addCookie(cookie);
        }

        request.setAttribute("hacker", user);
        request.setAttribute("logs", new LogHandler());
        return "index";
    }

I've written a rule, but it's a bit flawed and doesn't fully check out the entire path of request->bytes.

my ql rules:

/**
 * @name Unsafe shiro deserialization
 * @kind path-problem
 * @id java/unsafe-shiro-deserialization
 */


import java
import semmle.code.java.dataflow.FlowSources
import DataFlow::PathGraph

predicate isDes(Expr arg){
    exists(MethodAccess des |
    des.getMethod().hasName("deserialize") 
    and
    arg = des.getArgument(0)
    )
}

class ShiroUnsafeDeserializationConfig extends TaintTracking::Configuration {
    ShiroUnsafeDeserializationConfig() { 
        this = "StrutsUnsafeDeserializationConfig" 
    }

    override predicate isSource(DataFlow::Node source) {
        source instanceof RemoteFlowSource
    }
    override predicate isSink(DataFlow::Node sink) {
        exists(Expr arg|
            isDes(arg) and
            sink.asExpr() = arg /* bytes */
        )
    }
}


from ShiroUnsafeDeserializationConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
where config.hasFlowPath(source, sink)
select sink, source, sink, "Unsafe shiro deserialization" ,source, "this user input"

[smowton]

This looks like it's working fine -- the source at getValue is detected, base64Decode is known to propagate taint, then Tools.deserialize is recognised as the sink. What's the problem?

[SummerSec]

I mean I want to check the whole path request->cookies->cookie->bytes

[SummerSec]

Is it feasible?

[SummerSec]

Make it more perfect

[smowton]

You don't see request in the path here because Cookie.getValue is a RemoteFlowSource. If you want to include those in the path, you would need isSource to be something like source.(DataFlow::ParameterNode).getType().(RefType).hasQualifiedName("javax.servlet.http", "HttpServletRequest"), and you would need to specify isAdditionalTaintStep so that getCookies taints its return value.

However there is no reason to do this unless you really need the HttpServletRequest instance the cookie came from -- generally assuming that Cookie.getValue is dangerous works in 95% of cases.

[SummerSec]

thanks, i think understand it . Yesterday ,You answered my other question too, thanks again!