diff options
| -rw-r--r-- | fw/doc/design.md | 7 | ||||
| -rw-r--r-- | fw/doc/todo.md | 241 | ||||
| -rw-r--r-- | fw/man/fw | 12 | ||||
| -rw-r--r-- | fw/src/ether.c | 24 | ||||
| -rw-r--r-- | fw/src/fw.c | 89 | ||||
| -rw-r--r-- | fw/src/netfs.c | 67 | ||||
| -rw-r--r-- | fw/src/rules.c | 36 |
7 files changed, 327 insertions, 149 deletions
diff --git a/fw/doc/design.md b/fw/doc/design.md index d6f3e55..1c78db1 100644 --- a/fw/doc/design.md +++ b/fw/doc/design.md @@ -4,7 +4,7 @@ two networks, or in front of one namespace.** Status: working and tested on the init-test VM, in all three modes. Not -yet fit to run on a machine you care about — see `todo.md`, item 1. +yet fit to run on a machine you care about — see `todo.md`. ## Why @@ -72,7 +72,10 @@ write, and the text comes back out of `dial(2)` — which is a far better diagnostic than a dropped packet. Because it proxies the *assembled* `/net` rather than synthesising a -tree, `cs` and `dns` come along for free. +tree, `cs` and `dns` come along for free. That is a convenience and a +hole in the same sentence: it is also why name resolution cannot be +refused in this mode, so a program with an empty rule set can still get +names looked up, which is exfiltration if you care about that. The real `/net` needs no second name and must not have one: any surviving path to it is a way around the filter. lib9p forks the server with diff --git a/fw/doc/todo.md b/fw/doc/todo.md index 97d1d71..e750674 100644 --- a/fw/doc/todo.md +++ b/fw/doc/todo.md @@ -1,103 +1,148 @@ # fw: open items -Ranked. Item 1 is the only thing between this and running it on a -machine you care about. - -## 1. A dead fw takes the network with it - -**Severity: blocks use.** - -Taking a card is destructive and is not undone. The `pkt` interface that -replaces it is `unbindonclose`, so when `fw` stops the interface goes and -the address goes with it. The machine is left with a card bound to -nothing and no network. - -Worse, `fw` cannot restart unaided: the address it would read off the -card is the address that just vanished. So it exits, and a supervisor -with `restart=always` would spin. - -`fw` tries to put the card back as it exits (`putback`, via `atexit` and -`threadnotify`). That covers an orderly stop. **It did not fire on a kill -in testing** and I did not chase it further — a cleanup that works -sometimes is worse than none, because you would trust it. - -The fix is probably not more note handling. Whatever restarts `fw` has to -be able to configure the card first, which means the address has to -survive somewhere `fw` does not own. That is a supervisor's job. - -This is also what blocks `svc` supervision (item 3), so one fix, two -payoffs. - -## 2. Broadcast handling is written but unwitnessed - -Broadcast and multicast are addressed directly rather than resolved: -`255.255.255.255` and the subnet broadcast to `ff:ff:ff:ff:ff:ff`, -`224/4` to `01:00:5e:...`, `ff00::/8` to `33:33:...`. Without this a -DHCP renewal would be ARPed for the gateway and unicast there, and the -lease would quietly never renew. - -The mapping is the standard one and normal traffic is unaffected, but -**I never managed to get a broadcast to cross `fw` to confirm it.** -Reviewed, not observed. - -Related and unfixed: `fw` reads the address once at startup, so a lease -that *changes* the address goes unnoticed until restart. - -## 3. fw daemonizes, so svc cannot supervise it - -The process you exec returns immediately and leaves the server behind. -`svc` would see an instant exit and, with `restart=always`, spin. - -Needs a foreground mode where the process started is the process that -stays. `svc`'s `ready=srv:name` fits: `fw -s fw.ether0` already posts to -`/srv`. - -`fwstart` is the wrong shape and should probably go. `svc` already does -dependency ordering and per-service supervision; `fwstart` re-implements -the loop in rc and then exits, so `svc` would be supervising a process -that has already gone. One service per card is the right shape. - -## 4. One card per fw, and rules cannot name a card - -Two cards means two `fw`s with two rule files. Tested and it works, but -a rule cannot say `ifc=ether0`, so one file cannot express different -policy for different cards. Wants repeatable `-e` and an `ifc=` -attribute, and those go together. - -## 5. Tflush is not implemented - -A request `fw` is blocked on cannot be abandoned, so killing a program -that is waiting for an inbound connection does not reach `fw`. Only -affects the namespace mode; nothing in packet mode blocks indefinitely. - -Doable: record the worker's pid against the Req, post an `interrupt` -note on flush, catch it with `threadnotify` so the syscall returns -`interrupted` rather than killing the proc. Maybe 80 lines. There is a -race that cannot be fully closed — between the syscall returning and the -handler clearing its entry, a note may already be in flight and land on -a worker that has moved on, failing an unrelated request. Rare, -unreproducible, and the reason to do it deliberately. - -## 6. Positional delete renumbers - -`delete n` counts lines of `rules`, so numbers shift after each delete -and `delete 3` twice removes two different rules. Inherent to positional -deletion — iptables has it too — but it should be said out loud. - -## 7. Untested at the edges - -- The gateway has only been tested between two synthetic stacks on one - machine. `run.sh` has `-gw`/`-lan` for a two-VM test; the client VM - was never built. -- No IPv6 traffic has been pushed through any mode. The code paths - exist and parse v6, but nothing has exercised them. -- No test with a real second NIC carrying real traffic. +Most of this came from a code review after the first commit. Where a +finding has been fixed it says so and how it was checked; where it has +not, it says what is actually true. + +## Fixed since the review + +1. **The ctl filter was a blacklist.** `headers` on a udp conversation + passed straight through, and the write queue is live from clone, so + three writes sent a datagram anywhere with no `connect` for a rule to + match. Now a whitelist: `bind`, `ttl`, `tos`, `ignoreadvice`, + `close`, `hangup`, `keepalive` pass; `connect` and `announce` are + rule-checked; everything else is refused by name. Verified — `headers` + and gre `raw` are refused, `ttl` still works, and `none.ndb` now means + what it says. +2. **`%M` was never installed**, so `fmtrules` emitted `ipmask=%M%` and + every ctl edit on a rule set containing `ip=` failed. Reproduced, then + fixed with `fmtinstall('M', eipfmt)`. Verified round-tripping. +3. **`parserules` built the new list in the globals** with no lock, so + relays walked an empty then half-built list during any reload. Now + built in locals. Verified: load, reload, a rejected file leaving the + old rules, and edits. +4. **A 64KB frame on a 32KB proc stack** in `etherwriteip` — the same + bug already fixed in `relay()` and left here. Now `Maxframe`. +5. **`putback`/`notehandler` were dead code** — `atexit` matches on the + registering pid and `_exits` never runs the handlers. Removed, and + every doc that claimed the card was put back is corrected. Nothing + puts it back; see item 1 below. +6. **Expired flows kept matching.** `flowlook` refreshed `last` without + checking the timeout, and reaping only happens on insert, so on an + idle firewall a dead flow passed traffic forever. +7. **The pkt interface claimed a 4096 MTU** from a 1514-byte card. Now + `mtu 1500`. Verified: `pkt0 maxtu 1500`. +8. **Ports were read out of non-first fragments**, so fragmented traffic + was denied and a crafted fragment could pass. Later fragments now + match on addresses and protocol only. +9. **Four holes in the filtered `/net`**: `/net/ndb` and `/net/log` were + writable (mode 0666 — reconfigure name service, or trace every + connection on the machine), and `ipifc/*/data` was readable, which on + a machine also running `fw -e` is the packet stream itself. All + closed, reads still work. Verified. +10. **Control files were world-writable.** Now 0600/0440 and owned by + the user running `fw` — testing caught that owning them as a + nonexistent user "fw" locked out the administrator too. +11. **`delete 0` and `delete foo`** appended a rule reading `<nil>`. Now + refused. +12. **A dead relay left half a firewall** — one direction unfiltered, + nothing to notice. Now `threadexitsall`. +13. **IPv6 unicast under `-e`** was dropped in silence. It is still not + implemented — neighbour discovery is missing — but it now says so + once instead of pretending. + +## Still open + +### 1. Nothing puts the card back + +Taking a card is destructive and is not undone. The pkt interface is +`unbindonclose`, so when `fw` stops the interface and the address both +go, leaving the card bound to nothing and the machine with no network. +`fw` cannot restart unaided either: the address it would read off the +card is the address that just vanished. + +The cleanup that claimed to handle this was dead code and has been +removed. The answer is not more note handling — it has to be something +that outlives `fw`, which means the supervisor, with the address stored +where `fw` does not own it. This also blocks `svc` supervision. + +### deny=in ip=... silently never matches in namespace mode + +An `announce` sets `anyip`, and `matchrule` skips rules naming an +address, so a peer-address inbound rule is a no-op there while working +at the packet layer. It is a reasoned decision, but by this project's +own standard it should either be enforced at listen time (check +`remote`, hang up) or refused at startup in that mode. + +## Worth fixing + +- **`promiscuous` injects neighbours' unicast into the protected + stack.** `ethermux` already delivers frames addressed to us plus + multicast and broadcast; promiscuous only adds other machines' + traffic, into the rules, the stats and the flow table. It is needed + for multicast reception, so keep it and filter on the destination MAC + in `etherin`: accept `ourmac` or `d[0]&1`. +- **Control files are world-writable.** `ctl` and `rules` at 0666 means + any user can rewrite the firewall. 0660 or 0600. +- **Use-after-free of the matched rule.** `matchrule` returns `*rp` + after dropping `rulelock` and callers read `rule->log` outside it. + Copy the flag out under the lock. +- **`learnaddr` truncates the routing table** — `buf[1024]`, `lines[8]`, + against a table that routinely exceeds both, so the default route can + be missed. +- **`fmtrules` truncates silently at 64K**, so a large rule set loses + rules on every ctl edit and on save. Detect and fail. +- **`delete 0` / `delete foo`** falls into the append branch with a nil + argument and appends a rule reading `<nil>`. +- **A dead relay proc leaves a half-firewall** — the proc returns, the + others carry on, one direction is permanently dead with nothing to + notice. `threadexitsall` is the fail-closed answer. +- **Static error buffers race under `srvrelease`** — `netfs.c` and + `rules.c` both return `static char err[128]` while running multi-proc. + Worst case is a wrong diagnostic, which is the thing namespace mode + exists to produce. +- **`revalidate` inflates hit counts** — it calls `matchrule`, so + `stats` counts rule-change re-checks as decisions about traffic. + +## Smaller + +- `relay()` and `permitted()` are the same decision logic twice with + divergent log text; the gateway path should call `permitted()`. +- The `reclaim` comment is orphaned above `putback`. +- `putback`'s "the ctl fd must stay open" is wrong — + `ethermedium.unbindonclose` is 0. +- `fswalk1` does not reject names containing `/`. +- `fwstart`'s `mntgen` inherits the caller's fds — the trap CLAUDE.md + documents. + +## Still open from before + +- **Broadcast handling is written but never observed.** The mapping is + standard and normal traffic is unaffected, but no broadcast has been + seen crossing `fw`. +- **fw daemonizes**, so `svc` cannot supervise it. Needs a foreground + mode; `ready=srv:name` fits, since `-s` already posts to `/srv`. + `fwstart` is the wrong shape and should probably go. +- **One card per fw, and rules cannot name a card.** Wants repeatable + `-e` and an `ifc=` attribute together. +- **Tflush is not implemented.** Only affects namespace mode. ~80 lines, + with a race that cannot be fully closed. +- **Positional `delete n` renumbers.** +- **Never tested**: the gateway with two real VMs, any IPv6 traffic, a + real second NIC carrying traffic. + +## Docs that are now wrong + +- `design.md`, `todo.md` and `man/fw` all said `fw` "tries to put the + card back as it exits, which covers an orderly stop". It covers + nothing — see 5. +- `man/fw` SYNOPSIS shows `-a` as required with `-e`; it is optional. +- `man/fw` says an empty rule file means no network at all; see 1. +- `design.md` presents "cs and dns come along for free" as a benefit. It + is also why DNS cannot be blocked in namespace mode, which belongs + next to it. ## Deliberately not doing -**NAT.** See design.md. A 9front client imports `/net`; anything else -behind an IPv4 gateway with one address is the only case that needs it, -and that case can wait for someone who actually has it. - -**Rate limiting, fragment logic, ICMP type matching, deep IPv6.** Scope, -not difficulty. The useful firewall is the one that ships. +**NAT**, rate limiting, fragment reassembly, ICMP type matching, deep +IPv6. Scope, not difficulty. @@ -13,8 +13,8 @@ fw \- firewall .br .RB [ -e .I ether -.B -a -.IR addr / mask +.RB [ -a +.IR addr / mask ] .RB [ -g .IR gateway ]] .br @@ -455,12 +455,12 @@ one with or the card configured again with .IR ipconfig (8) first. -.I Fw -tries to put the card back as it exits, which covers an orderly stop but -not a kill. +Nothing puts the card back. Whatever restarts .I fw -should be prepared to configure the card first. +must configure the card first, with the address it had, which +.I fw +no longer knows. .PP The first packet to an unresolved next hop is dropped while .I fw diff --git a/fw/src/ether.c b/fw/src/ether.c index 54f5068..dd20971 100644 --- a/fw/src/ether.c +++ b/fw/src/ether.c @@ -38,6 +38,7 @@ enum Arplen = 28, Eminlen = 60, /* ethernet minimum frame, devether enforces it */ + Maxframe = Ehdrlen + 16*1024, Arpreq = 1, Arpreply = 2, @@ -323,11 +324,16 @@ etherisip(uchar *f, int n) int etherwriteip(uchar *p, int n, uchar *mask) { - uchar f[Ehdrlen + 64*1024], dst[IPaddrlen], hop[IPaddrlen]; + /* + * On a proc stack, which libthread keeps small: 64K here + * overran it and corrupted the data segment. pktmedium's maxtu + * is 4096, so this is already generous. + */ + uchar f[Maxframe], dst[IPaddrlen], hop[IPaddrlen]; uchar net[IPaddrlen], ournet[IPaddrlen], mac[Eaddrlen]; int type, len; - if(n < 20 || n > 64*1024 - Ehdrlen) + if(n < 20 || n > Maxframe - Ehdrlen) return -1; switch(p[0] >> 4){ case 4: @@ -355,6 +361,20 @@ etherwriteip(uchar *p, int n, uchar *mask) ipmove(hop, dst); if(!arpget(hop, mac)){ + /* + * v6 has no ARP; resolving a neighbour needs ICMPv6 + * solicitation, which is not implemented. Say so once, + * rather than dropping every v6 unicast in silence. + */ + if(!isv4(hop)){ + static int said; + + if(!said++) + fprint(2, "fw: cannot resolve %I: " + "IPv6 neighbour discovery is not implemented, " + "so v6 unicast is dropped\n", hop); + return -1; + } if(etherdebug) fprint(2, "arp: no entry for %I, dropping and asking\n", hop); arpask(hop); diff --git a/fw/src/fw.c b/fw/src/fw.c index 4f94090..a136194 100644 --- a/fw/src/fw.c +++ b/fw/src/fw.c @@ -48,6 +48,7 @@ struct Pkt uchar dst[IPaddrlen]; int sport; int dport; + int frag; /* a later fragment: no ports in it */ int verb; }; @@ -130,6 +131,15 @@ flowlook(int proto, uchar *src, uchar *dst, int sport, int dport, long now) h = flowhash(proto, src, dst, sport, dport); for(f = flowtab[h]; f != nil; f = f->next) if(flowis(f, proto, src, dst, sport, dport)){ + /* + * An expired flow must not match. Reaping only + * happens when a flow is added, so on a quiet + * firewall nothing is ever reaped and a flow that + * timed out long ago would keep passing traffic, + * refreshing itself on every packet. + */ + if(now - f->last > flowtimeout(f->proto)) + return 0; f->last = now; return 1; } @@ -295,6 +305,13 @@ wireup(Wire *w) if(fprint(w->cfd, "bind pkt") < 0) sysfatal("%s: bind pkt: %r", w->net); + /* + * pktmedium claims 4096 with no link header; a card is 1514 with + * 14. Left alone the protected stack emits packets the card + * refuses, and only remote peers capping the MSS hide it. + */ + if(fprint(w->cfd, "mtu 1500") < 0) + fprint(2, "fw: %s: cannot set mtu: %r\n", w->net); if(fprint(w->cfd, "add %s %s", w->addr, w->mask) < 0) sysfatal("%s: add %s %s: %r", w->net, w->addr, w->mask); @@ -333,6 +350,15 @@ parsepkt(uchar *b, int n, Pkt *p) p->proto = b[9]; v4tov6(p->src, b + 12); v4tov6(p->dst, b + 16); + /* + * Only the first fragment carries the transport header. + * Reading one out of a later fragment gives payload bytes + * as ports, which both loses the traffic - it matches no + * flow - and lets a crafted fragment whose bytes happen to + * match an open flow through. + */ + if((nhgets(b + 6) & 0x1FFF) != 0) + p->frag = 1; t = b + hl; n -= hl; break; @@ -349,7 +375,7 @@ parsepkt(uchar *b, int n, Pkt *p) return; } - if((p->proto == 6 || p->proto == 17) && n >= 4){ + if(!p->frag && (p->proto == 6 || p->proto == 17) && n >= 4){ p->sport = nhgets(t); p->dport = nhgets(t + 2); } @@ -382,7 +408,7 @@ relay(Wire *from, Wire *to, int verb) if((n = read(from->dfd, buf, Maxpkt)) <= 0){ fprint(2, "fw: %s: read failed: %r\n", from->side); syslog(0, "fw", "%s: read failed: %r", from->side); - return; + threadexitsall("wire"); } parsepkt(buf, n, &p); @@ -818,8 +844,15 @@ fswrite(Req *r) err = editrules(arg, 1, 0); else if(strcmp(buf, "append") == 0) err = editrules(arg, 0, 0); - else if(strcmp(buf, "delete") == 0) - err = editrules(nil, 0, atoi(arg)); + else if(strcmp(buf, "delete") == 0){ + int nr; + + nr = atoi(arg); + if(nr < 1) + err = "delete wants a rule number, from 1"; + else + err = editrules(nil, 0, nr); + } else err = "unknown command; read ctl for the list"; @@ -854,14 +887,21 @@ rulesdidchange(void) static void servectl(char *mtpt, char *srvname) { + char *user; File *root; - fs.tree = alloctree("fw", "fw", DMDIR|0555, nil); + /* + * Owned by whoever is running fw, not by a user called "fw" that + * does not exist: with 0600 that locked out the administrator as + * effectively as everyone else. + */ + user = getuser(); + fs.tree = alloctree(user, user, DMDIR|0555, nil); root = fs.tree->root; - closefile(createfile(root, "ctl", "fw", 0666, (void*)Qctl)); - closefile(createfile(root, "rules", "fw", 0666, (void*)Qrules)); - closefile(createfile(root, "flows", "fw", 0444, (void*)Qflows)); - closefile(createfile(root, "stats", "fw", 0444, (void*)Qstats)); + closefile(createfile(root, "ctl", user, 0600, (void*)Qctl)); + closefile(createfile(root, "rules", user, 0600, (void*)Qrules)); + closefile(createfile(root, "flows", user, 0440, (void*)Qflows)); + closefile(createfile(root, "stats", user, 0440, (void*)Qstats)); threadpostmountsrv(&fs, srvname, mtpt, MREPL); } @@ -947,9 +987,14 @@ etherin(void *a) buf = emalloc(Maxpkt); for(;;){ if((n = read(efd, buf, Maxpkt)) <= 0){ + /* + * One relay stopping would leave the other running + * and that direction unfiltered, with nothing to + * notice. Take the whole firewall down instead. + */ fprint(2, "fw: %s: read the card: %r\n", w->side); syslog(0, "fw", "stopped reading the card: %r"); - return; + threadexitsall("card"); } if(debug > 1) fprint(2, "wire: %d bytes type %.4ux\n", n, (buf[12]<<8)|buf[13]); @@ -977,7 +1022,7 @@ etherout(void *a) if((n = read(w->dfd, buf, Maxpkt)) <= 0){ fprint(2, "fw: %s: read the stack: %r\n", w->side); syslog(0, "fw", "stopped reading the stack: %r"); - return; + threadexitsall("stack"); } if(permitted(buf, n, Vout, &p)) etherwriteip(buf, n, ethermask); @@ -1057,6 +1102,19 @@ learnaddr(char *net, char *dev, char *addr, int naddr, char *gw, int ngw) * it: a live fw would still be holding the card we are about to take. */ /* + * Undo a previous fw that died. + * + * An IP stack outlives the program that configured it, so a fw that is + * killed leaves its pkt interface behind, holding the address, with + * nothing on the other end of it. The machine has no network until + * someone unpicks that by hand, and the next fw to start makes a second + * interface with the same address and routes that could go to either. + * + * So before taking anything, throw away any pkt interface already + * carrying the address we are about to use. Nothing else can have made + * it: a live fw would still be holding the card we are about to take. + */ +/* * Put the card back. * * Taking a card is destructive: the stack loses it, and the pkt @@ -1248,6 +1306,7 @@ threadmain(int argc, char **argv) fmtinstall('I', eipfmt); fmtinstall('V', eipfmt); fmtinstall('E', eipfmt); + fmtinstall('M', eipfmt); /* fmtrules prints masks with it */ rulepath = argv[0]; readrules(rulepath); @@ -1317,14 +1376,6 @@ threadmain(int argc, char **argv) ethersetaddr(ip, gw, haveg); reclaim(netmtpt, abuf); - backdev = etherdev; - backaddr = abuf; - backmask = m; - backnet = netmtpt; - backgw = ethergw; - atexit(putback); - threadnotify(notehandler, 1); - takecard(netmtpt, etherdev); prot = emalloc(sizeof *prot); diff --git a/fw/src/netfs.c b/fw/src/netfs.c index c87412d..f28485f 100644 --- a/fw/src/netfs.c +++ b/fw/src/netfs.c @@ -121,11 +121,31 @@ hidden(char *path) static char* protect(char *path, int mode) { + char *p; + if(hidden(path)) return "fw: does not exist"; if(strcmp(path, "ipifc/clone") == 0) return "fw: interface creation denied"; - if(under(path, "ipifc") || under(path, "iproute") || under(path, "arp")) + + /* + * An interface's data file is a wire. On a machine that is also + * running fw -e it is *the* wire, so reading it hands a filtered + * program the packet stream the filter exists to control. Not + * read-only: not at all. + */ + if(under(path, "ipifc")) + if((p = strrchr(path, '/')) != nil && strcmp(p+1, "data") == 0) + return "fw: denied"; + + /* + * ndb is the machine's configuration and log is a trace of every + * connection on it, both mode 0666. Writing the first + * reconfigures name service for everyone; reading the second + * after enabling it watches the whole machine. + */ + if(under(path, "ipifc") || under(path, "iproute") || under(path, "arp") + || under(path, "ndb") || under(path, "log")) if((mode & 3) != OREAD) return "fw: read-only under fw"; return nil; @@ -181,6 +201,22 @@ ctlproto(char *path) * connect takes addr!port with optional trailing fields; announce takes * a bare port, or addr!port with addr often "*". */ +/* + * Control messages that only change how this one conversation behaves, + * and so cannot reach the network by themselves. Everything outside + * this list is refused: see checkctl. + */ +static char *okverbs[] = { + "bind", /* the local address; announce is what opens */ + "ttl", + "tos", + "ignoreadvice", + "close", + "hangup", + "keepalive", + nil, +}; + static char* checkctl(char *proto, char *msg, long n) { @@ -188,7 +224,7 @@ checkctl(char *proto, char *msg, long n) static char err[128]; uchar ip[IPaddrlen], mask[IPaddrlen]; Rule *rule; - int nf, na, verb, anyip, port, lport; + int nf, na, verb, anyip, port, lport, i; if(n <= 0) return nil; @@ -203,8 +239,31 @@ checkctl(char *proto, char *msg, long n) verb = Vout; else if(strcmp(f[0], "announce") == 0) verb = Vin; - else - return nil; /* hangup, ttl, keepalive: not policy */ + else{ + /* + * Anything else is refused unless it is known to be + * harmless. Letting unknown control messages through was + * a hole, not a convenience: "headers" on a udp + * conversation turns it into one that carries its own + * destination, and the write queue is live from the + * moment it is cloned, so three writes send a datagram + * anywhere with no connect for a rule to match. gre has + * "raw" and "forward"; rudp and icmpv6 have "headers" + * too. + * + * This is the same argument the rule parser already makes + * about attributes it does not recognise, applied to the + * place it was not. + */ + for(i = 0; okverbs[i] != nil; i++) + if(strcmp(f[0], okverbs[i]) == 0) + return nil; + syslog(0, "fw", "deny %s %s: control message not permitted", + proto, f[0]); + snprint(err, sizeof err, + "fw: %s: not a permitted control message", f[0]); + return err; + } if(nf < 2) return nil; /* malformed; let the kernel say so */ diff --git a/fw/src/rules.c b/fw/src/rules.c index c0df82d..55de9cb 100644 --- a/fw/src/rules.c +++ b/fw/src/rules.c @@ -30,7 +30,6 @@ Rule *rules; void (*rulechanged)(void); static Lock rulelock; -static Rule *lastrule; static char *rulefile; static char *parseerr; static jmp_buf parsejmp; @@ -135,7 +134,7 @@ verbof(char *s) * a firewall gets to do. */ static void -addrule(Ndbtuple *t, int nr) +addrule(Ndbtuple *t, int nr, Rule **head, Rule **tail) { char *ip, *mask, *p, abuf[64]; Rule *r; @@ -206,28 +205,34 @@ addrule(Ndbtuple *t, int nr) r->anyip = 0; } - if(lastrule == nil) - rules = r; + if(*head == nil) + *head = r; else - lastrule->next = r; - lastrule = r; + (*tail)->next = r; + *tail = r; } /* * Parse without installing. Returns the new list, or nil with *err set. * An empty file is a valid rule set: it denies everything. */ +/* + * Build the new list in locals. Earlier this borrowed the globals and + * put them back afterwards, which meant that for the length of a reload + * the relay procs - which take rulelock, a lock this never held - were + * walking a list that was first empty and then half built. Every packet + * in that window was judged against a partial rule set, and a parse + * failure freed nodes a relay might still have been holding. + */ Rule* parserules(char *file, char **err) { - Rule *new, *save, *savelast; + Rule *head, *tail; Ndbtuple *t; Ndb *db; int nr; - save = rules; - savelast = lastrule; - rules = lastrule = nil; + head = tail = nil; rulefile = file; parseerr = nil; @@ -236,30 +241,25 @@ parserules(char *file, char **err) snprint(eb, sizeof eb, "%s: %r", file); *err = eb; - rules = save; - lastrule = savelast; return nil; } parsing = 1; if(setjmp(parsejmp) == 0){ for(nr = 1; (t = ndbparse(db)) != nil; nr++){ - addrule(t, nr); + addrule(t, nr, &head, &tail); ndbfree(t); } } parsing = 0; ndbclose(db); - new = rules; - rules = save; - lastrule = savelast; if(parseerr != nil){ - freerules(new); + freerules(head); *err = parseerr; return nil; } *err = nil; - return new; + return head; } void |