NetLay: Letting the robots audit it

Five reviewers, one job each#

Twenty seven posts of building, and the only person who has ever reviewed this code is me. I know what I meant, which makes me the worst possible reviewer of it.

So I ran five automated reviews in parallel, each with one job and nothing else to think about. Security and permissions. Correctness. Usability. Data safety and operations. Performance with around eight hundred and fifty devices on a map. Each one got its own throwaway copy of the application running on a scratch data folder, read only, so none of them could tread on each other or on anything of mine.

One rule for every finding. Give me the file and the line, and either reproduce it or label it as likely, or drop it. A review that produces a list of things that might be wrong somewhere is a list I will never work through.

About seventy five findings came back. This post is the ones that mattered.

The one that meant stop.#

The route that serves the web application takes whatever path was requested and joins it onto the folder the app lives in, and serves the file if it exists.

Read that again, because I did, several times.

There is no check that the result is still inside that folder. A request with enough encoded dot dot slashes in it walks straight out and fetches any file on the machine, with no sign in required.

In local mode that is bad. In shared mode, which I added only five posts ago and was quite pleased with, it means anybody on the network can fetch the database. And the session signing key, which sits in the same data folder. And with that key you can forge yourself an admin cookie.

The fix is small: resolve the path and refuse it unless it is still inside the application folder. The lesson is in how it was tested. The test client tidies up paths like that before the request is even made, so it cannot reproduce the bug and would have cheerfully told me the fix was unnecessary. It has to be tested with a raw request that sends the path exactly as written.

A test framework being helpful is a wonderful thing right up until the helpfulness is hiding the hole.

Sessions that outlive their owner.#

The session cookie held a user id and nothing else. SQLite reuses row ids. So delete a user, create another, and the deleted person's cookie now belongs to the new account.

That was reproduced as a full admin takeover, which is the sort of sentence you do not enjoy reading about your own work.

Cookies now carry a random per-user key as well as the id. Disabling an account, changing a password or an admin reset all change that key, which signs that person out everywhere, which is the behaviour anybody would expect and which I had simply never implemented. Sign in throttling and a lock on the first run setup went in at the same time.

Permissions written for one person.#

This is the theme of the whole review, and it is not a bug so much as a wrong assumption that spread.

NetLay was built as a tool for one person, and then I added accounts and sharing and did not go back through everything that had been written before. So any signed in user could run the DHCP tracker, which rewrites addresses on maps they are not even allowed to edit. Or delete a VLAN used by every layout in the system. Or edit the catalog, or start a scan from the server, or read the SNMP community string.

The importer had a sharper version of the same thing. It trusted the switch ids the browser sent back, so one editor could overwrite a switch on somebody else's layout.

The pattern I settled on afterwards: shared lists can be added to by editors and changed by admins, and anything that touches another layout or reaches out onto the network needs the right role. That is three sentences and it should have been written down the day accounts arrived.

Smaller holes, same shape.#

An SVG floor plan can contain script, and floor plans were served without a sign in. Uploads need authentication now and are served with a policy that stops anything inside them running.

A discovery scan of everything would build four billion addresses in memory before getting round to checking the limit.

A malformed DHCP lease file made the parser quadratic. A device link could be a javascript: URL. An address beginning with a minus sign went to the ping command as an argument.

Backups, finally.#

The data safety review opened with the observation that no copy of the database existed anywhere, ever, under any circumstances.

That is now: one backup before every start up, so a failed upgrade can be undone, one a day, and a button for admins. The last five start up copies, fourteen daily ones and ten manual ones are kept.

The photo clean up needed fixing too. It deleted any file the database did not know about, which is correct until somebody puts a different database in place, at which point it deletes every photo they own. It moves them to a trash folder for thirty days now, and refuses to run at all if most of the photos suddenly look orphaned, because that is not a clean up, that is a disaster in progress.

What I have not fixed yet#

Three more phases of findings are still open and I would rather say so than imply the list is closed.

Sites leak into each other, because leases, ARP results, discovery and VLAN names are all global. The IP plan falls over on IPv6. Around three percent of MAC addresses read over SNMP come back mangled. Dragging a big selection takes the best part of eight seconds a move. Ping now waits for a full cycle. The history roll up locks the database. And you still cannot move a device to a different switch from the interface at all.

Next time, the first of those, which turned out to be the one with a design decision hiding inside it.

NetLayNetworking