The folks from Radically Open Security recently completed their security audit. This was sponsored by the NGI0 Entrust Fund via the NLnet foundation. My deepest appreciation for this opportunity.
The audit uncovered four minor findings, but no huge security issues. I remain confident in my design and approach. It is good to have a second pair of eyes reviewing these. The following is a short summary of these findings.
Inappropriate file mode
The data in vdir storage is marked executable. Items themselves are well-escaped, making it difficult to make use of this, however the display name of a collection is not, so an attacker controlling the display name can effectively control the contents of an executable file.
This was a mistake on my part; I used the constant Mode::RWXU, which was not
what I’d intended. I replaced with with 0o600, which is much easier to read.
In theory, an attacker with access to a remote calendar could have injected a malicious binary and tricked a local user to double click on the file, triggering the execution.
The issue by itself is quite minor, but could have been exploited with a combination of (1) write access to a remote CalDav or CardDav instance and (2) social engineering.
This issue was fixed in 302be5cc7b2c755b91311cc84a89f8f36f8733cd.
Potential panic in error handling
The application will panic if a storage provider such as a WebDAV server returns a malformed item.
Vdirsyncer could be made to panic if a WebDAV server served a completely empty
file. I fixed this issue in the vparser library, and included a unit test to
avoid future regressions.
This issue could have resulted in a remote server performing a denial of service on vdirsyncer by making it crash continuously. No data corruption or data leak could have occurred because of this.
This issue was fixed in a2a25d881c00f23b5dd60e3c7752c327ca11806a.
Following symlinks
Symlinks are followed by the vdirsyncer implementation even if they fall outside the target sync directory.
Vdirsyncer performs atomic writes by creating a new file and overwriting the previous one. Because of this, it could never write to a location outside the vdir, even in the presence of a symlink.
The only scenario where this can potentially be exploited, is where Alice has write permissions to a vdir, but Bob runs vdirsyncer on it. In such a scenario, Alice could potentially leak files which are only readable by Bob by uploading them to a remote storage.
The most likely course of action here will be to treat symlinks as unreadable files.
This is currently tracked in https://todo.sr.ht/~whynothugo/vdirsyncer-rs/94
No data validation
An item in a vdir can be replaced with any data, and vdirsyncer will attempt to submit it to the sync target.
This is an intentional design choice; if a user has data that is technically invalid, we still want to permit them to synchronise it in any direction. If we prohibit synchronising invalid data, users may become incapable of operating on this data in order to fix the invalidity.
It is also possible that a user is using a calendar client which handles this “technically invalid” data just fine, and we don’t want to get in the way of users synchronising this.
While I don’t consider this an issue in itself, the fact that it was reported as a security finding reveals that this behaviour was not properly documented. I had added a mention on this topic to the documentation so the behaviour is clear for all users.
Other findings
Early during the audit some security issues were found in dependencies. These were unlikely to be exploitable via vdirsyncer, but the dependencies where promptly updated anyway.
I also added rules to the several of the libraries that fall under this project to prohibit unsafe code. These are higher level libraries, and there’s no need to escape Rust’s typical safety rules. Some dependencies, however, contain unsafe code. This is inevitable for lower level dependencies.
The auditor and I also had some chats to confirm that my approach on TLS configuration made sense and that there were no gaps in my reasoning. Their general feedback mentioned that […] the simplicity of the design and choice of Rust that simultaneously reduces attack surface and averts common memory- safety bugs.
Closing words
Security is an ongoing process. While no greater issues have been found at this time, work on vdirsyncer continues, and the security status of the project needs to be reviewed at a future time.