Skip to content

Commit

Permalink
fix GHSA-f54q-j679-p9hh: reflected-XSS in cookie-setters;
Browse files Browse the repository at this point in the history
it was possible to set cookie values which contained newlines,
thus terminating the http header and bleeding into the body.

We now disallow control-characters in queries,
but still allow them in paths, as copyparty supports
filenames containing newlines and other mojibake.

The changes in `set_k304` are not necessary in fixing the vulnerability,
but makes the behavior more correct.
  • Loading branch information
9001 committed Jul 23, 2023
1 parent 335fcc8 commit 007d948
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 2 deletions.
31 changes: 29 additions & 2 deletions copyparty/httpcli.py
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,15 @@ def run(self) -> bool:
vpath, arglist = self.req.split("?", 1)
self.trailing_slash = vpath.endswith("/")
vpath = undot(vpath)

zs = unquotep(arglist)
m = self.conn.hsrv.ptn_cc.search(zs)
if m:
hit = zs[m.span()[0] :]
t = "malicious user; Cc in query [{}] => [{!r}]"
self.log(t.format(self.req, hit), 1)
return False

for k in arglist.split("&"):
if "=" in k:
k, zs = k.split("=", 1)
Expand Down Expand Up @@ -488,6 +497,9 @@ def run(self) -> bool:
pex: Pebkac = ex # type: ignore

try:
if pex.code == 999:
return False

post = self.mode in ["POST", "PUT"] or "content-length" in self.headers
if not self._check_nonfatal(pex, post):
self.keepalive = False
Expand Down Expand Up @@ -586,6 +598,14 @@ def send_headers(
for k, zs in list(self.out_headers.items()) + self.out_headerlist:
response.append("%s: %s" % (k, zs))

for zs in response:
m = self.conn.hsrv.ptn_cc.search(zs)
if m:
hit = zs[m.span()[0] :]
t = "malicious user; Cc in out-hdr {!r} => [{!r}]"
self.log(t.format(zs, hit), 1)
raise Pebkac(999)

try:
# best practice to separate headers and body into different packets
self.s.sendall("\r\n".join(response).encode("utf-8") + b"\r\n\r\n")
Expand Down Expand Up @@ -785,7 +805,7 @@ def handle_get(self) -> bool:
path_base = os.path.join(self.E.mod, "web")
static_path = absreal(os.path.join(path_base, self.vpath[5:]))
if not static_path.startswith(path_base):
t = "attempted path traversal [{}] => [{}]"
t = "malicious user; attempted path traversal [{}] => [{}]"
self.log(t.format(self.vpath, static_path), 1)
self.tx_404()
return False
Expand Down Expand Up @@ -3077,7 +3097,14 @@ def tx_mounts(self) -> bool:
return True

def set_k304(self) -> bool:
ck = gencookie("k304", self.uparam["k304"], self.args.R, False, 86400 * 299)
v = self.uparam["k304"].lower()
if v == "y":
dur = 86400 * 299
else:
dur = None
v = "x"

ck = gencookie("k304", v, self.args.R, False, dur)
self.out_headerlist.append(("Set-Cookie", ck))
self.redirect("", "?h#cc")
return True
Expand Down
3 changes: 3 additions & 0 deletions copyparty/httpsrv.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import base64
import math
import os
import re
import socket
import sys
import threading
Expand Down Expand Up @@ -138,6 +139,8 @@ def __init__(self, broker: "BrokerCli", nid: Optional[int]) -> None:
zs = os.path.join(self.E.mod, "web", "deps", "prism.js.gz")
self.prism = os.path.exists(zs)

self.ptn_cc = re.compile(r"[\x00-\x1f]")

self.mallow = "GET HEAD POST PUT DELETE OPTIONS".split()
if not self.args.no_dav:
zs = "PROPFIND PROPPATCH LOCK UNLOCK MKCOL COPY MOVE"
Expand Down
1 change: 1 addition & 0 deletions copyparty/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ def sunpack(fmt: bytes, a: bytes) -> tuple[Any, ...]:
500: "Internal Server Error",
501: "Not Implemented",
503: "Service Unavailable",
999: "MissingNo",
}


Expand Down

0 comments on commit 007d948

Please sign in to comment.