Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

Add FastHTTP web framework #538

Closed
wants to merge 3 commits into from
Closed

Conversation

gagliardetto
Copy link
Contributor

@gagliardetto gagliardetto commented May 9, 2021

Part of github/securitylab#335

This PR adds models for the github.com/valyala/fasthttp web framework.

CodeQL Module Summary for FastHTTP:

Packages:

  • github.com/valyala/fasthttp@v1.23.0

Model kind TaintTracking:


Model kind HTTP::Redirect:


Model kind HTTP::HeaderWrite:


Model kind HTTP::ResponseBody:


Model kind UntrustedFlowSource:

@gagliardetto gagliardetto requested a review from a team as a code owner May 9, 2021 21:17
@gagliardetto
Copy link
Contributor Author

gagliardetto commented May 9, 2021

TODO:

  • The taint-tracking seems to not work from result to receiver. (Edit: I had forgotten to add the Link class for the tests).
  • Would it be possible to limit the inline tests to specific files? (to avoid cross-contamination) (Edit: done)
  • I still need to add HTTP::ClientRequest models.

@smowton
Copy link
Contributor

smowton commented May 13, 2021

  • Treating Request, RequestCtx, Response, ResponseCtx as both containers that propagate taint and sources/sinks seems unnecessary, considering in 99.999% of cases these types will not be used like containers.
  • URI.Scheme() is usually not dangerous, since the request got here in the first place somehow (unless you have reason to believe FastHTTP wouldn't itself check this is something sensible like http)
  • I see some functions with taint models that escape HTML, for example. These should be sanitisers, or just not modelled (simpler at the expense of perhaps some FNs if an HTML escaping function is used in a context where a different form of escaping was needed)

@smowton
Copy link
Contributor

smowton commented Jun 22, 2021

(deleted, turned out to be nonsense)

@gagliardetto
Copy link
Contributor Author

URI.Scheme() is usually not dangerous, since the request got here in the first place somehow (unless you have reason to believe FastHTTP wouldn't itself check this is something sensible like http)

It doesn't.

@gagliardetto
Copy link
Contributor Author

I believe that if I assumed that web frameworks do check that to be a sensible value, I would be wrong in most cases.

@gagliardetto
Copy link
Contributor Author

Server:

package main

import (
	"fmt"
	"log"

	"github.com/valyala/fasthttp"
)

func main() {
	listenAddr := "127.0.0.1:8087"

	requestHandler := func(ctx *fasthttp.RequestCtx) {
		fmt.Fprintf(ctx,
			"Hello, world! Request protocol is %q, scheme is %q",
			ctx.Request.Header.Protocol(),
			ctx.Request.URI().Scheme(),
		)
	}

	if err := fasthttp.ListenAndServe(listenAddr, requestHandler); err != nil {
		log.Fatalf("error in ListenAndServe: %s", err)
	}
}

Let's send a request:

printf "HELLO caramel://whatever DONUT/123\r\n\r\n" | nc 127.0.0.1 8087

image

@sauyon
Copy link
Contributor

sauyon commented Jul 15, 2021

I'd be inclined to exclude scheme anyway, because the most of the time it's unlikely to cause any injection vulnerabilities, and for something like reflected XSS it's not a vector.

That said, the other alternative would be excluding it manually from XSS.

@gagliardetto
Copy link
Contributor Author

Well, yes. This only works if there isn't a browser involved.

@gagliardetto
Copy link
Contributor Author

Scheme and protocol; anything else to be remove?

Copy link
Contributor

@sauyon sauyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've re-reviewed this since it's been a while.

out.isResult()
)
or
// signature: func AppendHTMLEscape(dst []byte, s string) []byte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add these as sanitizers for XSS.

"Qualifier": {
"Path": "github.com/valyala/fasthttp",
"Version": "v1.23.0",
"ID": "Function-AppendHTTPDate",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a bit nicer if the disabled selectors were removed from the json.

Comment on lines +32 to +33
inp.isParameter(0) and
out.isResult()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should include these; most of the time the contents of the dst slices will be overwritten, right?

out.isReceiver()
or
// signature: func (*Args).String() string
this.hasQualifiedName(packagePath(), "Args", "String") and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need to model String methods.

this = out.getExitNode(mtd.getACall()) and
mtd.hasQualifiedName(packagePath(), receiverName, methodName)
|
receiverName = "Args" and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think taint should originate from Args, Cookie, RequestHeader, or URI; probably it should propagate from the relevant fields of the Request.

@JarLob
Copy link

JarLob commented Nov 18, 2021

@gagliardetto any progress on this?

@gagliardetto
Copy link
Contributor Author

thanks for the ping, @JarLob

I am no longer working on this, and don't plan on resuming.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants