config: try to parse as ProtoJSON

[tomfitzhenry]

JSON is an easier to generate than TextProto in some environments, e.g. Jsonnet or Nix.

https://protobuf.dev/reference/cpp/api-docs/google.protobuf.util.json_util/

[robertswiecki]

Could we 'simply' accept both?

if (parseJson(...) == false) {
if (parseTextProto() == false) {
error();
}
}

Sth a'la (just better with logging):

diff --git a/config.cc b/config.cc
index 077c1b9..71ef9a3 100644
--- a/config.cc
+++ b/config.cc
@@ -24,6 +24,7 @@
 #include <fcntl.h>
 #include <google/protobuf/io/zero_copy_stream_impl.h>
 #include <google/protobuf/text_format.h>
+#include <google/protobuf/util/json_util.h>
 #include <stdio.h>
 #include <sys/mount.h>
 #include <sys/personality.h>
@@ -310,24 +311,29 @@ static void logHandler(
 bool parseFile(nsjconf_t* nsjconf, const char* file) {
 	LOG_D("Parsing configuration from '%s'", file);
 
-	int fd = TEMP_FAILURE_RETRY(open(file, O_RDONLY | O_CLOEXEC));
-	if (fd == -1) {
+	google::protobuf::SetLogHandler(logHandler);
+
+	std::ifstream ifs(file);
+	if (!ifs.is_open()) {
 		PLOG_W("Couldn't open config file '%s'", file);
 		return false;
 	}
 
-	google::protobuf::SetLogHandler(logHandler);
-	google::protobuf::io::FileInputStream input(fd);
-	input.SetCloseOnDelete(true);
+	std::string conf((std::istreambuf_iterator<char>(ifs)), (std::istreambuf_iterator<char>()));
 
 	/* Use static so we can get c_str() pointers, and copy them into the nsjconf struct */
 	static nsjail::NsJailConfig nsc;
 
-	auto parser = google::protobuf::TextFormat::Parser();
-	if (!parser.Parse(&input, &nsc)) {
-		LOG_W("Couldn't parse file '%s' from Text into ProtoBuf", file);
-		return false;
+	auto status = google::protobuf::util::JsonStringToMessage(conf, &nsc);
+	if (!status.ok()) {
+		LOG_W("Couldn't parse file '%s' from JSON into ProtoBuf", file);
+
+		if (!google::protobuf::TextFormat::ParseFromString(conf, &nsc)) {
+			LOG_W("Couldn't parse file '%s' from Text into ProtoBuf", file);
+			return false;
+		}
 	}
+
 	if (!parseInternal(nsjconf, nsc)) {
 		LOG_W("Couldn't parse the ProtoBuf from '%s'", file);
 		return false;

[tomfitzhenry]

Could we 'simply' accept both?

I wondered this, but I'm not familiar with all the intricacies of TextProto and ProtoJSON to know there aren't atypical config files that could parse as valid TextProto and ProtoJSON, and thus a fallback-parser would result in confusing semantics.

But, I'd guess any such atypical files were maliciously crafted. Meanwhile, I guess nsjail assumes the config file is trusted (else all bets are off).

I've updated this to PR to your suggestion (updated to only log if both parsers fail to recognize the file).

[tomfitzhenry]

Could you test what warnings are produced by this (because of google::protobuf::SetLogHandler).

Here's some test cases with the improved code.

Setup (to workaround NixOS not adhering to Linux FHS):

$ mkdir /tmp/bin
$ wget -O /tmp/bin/sh https://www.busybox.net/downloads/binaries/1.35.0-x86_64-linux-musl/busybox
$ chmod +x /tmp/bin/sh

Test cases

Degenerate

$ ./nsjail -C not-a-file
[W][2024-06-25T22:45:21+1000][17449] parseFile():326 Couldn't open config file 'not-a-file': No such file or directory
[F][2024-06-25T22:45:21+1000][17449] parseArgs():577 Couldn't parse configuration from 'not-a-file' file

✅

$ touch empty
$ ./nsjail -C empty
[E][2024-06-25T23:12:10+1000][18699] setupArgv():381 No command-line provided
[F][2024-06-25T23:12:10+1000][18699] main():334 Couldn't parse cmdline options

✅. This was initially surprising to me, but I see what's happened. An empty file parses as valid TextProto, and is then passed to parseInternal. Alas, there is no execBin, hence "No command-line provided". I checked with nsjail 3.4 and the behaviour is the same.

JSON

$ printf '{ "execBin": { "path": "/bin/sh" }, "mount": [ { "dst": "/bin", "src": "/tmp/bin", "isBind": true } ] }' > busybox-sh.json
$ ./nsjail -C busybox-sh.json
[I][2024-06-25T22:47:49+1000] Mode: STANDALONE_ONCE
[I][2024-06-25T22:47:49+1000] Jail parameters: hostname:'NSJAIL', chroot:'', process:'/bin/sh', bind:[::]:0, max_conns:0, max_conns_per_ip:0, time_limit:600, personality:0, daemonize:false, clone_newnet:true, clone_newuser:true, clone_newns:true, clone_newpid:true, clone_newipc:true, clone_newuts:true, clone_newcgroup:true, clone_newtime:false, keep_caps:false, disable_no_new_privs:false, max_cpus:0
[I][2024-06-25T22:47:49+1000] Mount: '/' flags:MS_RDONLY type:'tmpfs' options:'' dir:true
[I][2024-06-25T22:47:49+1000] Mount: '/tmp/bin' -> '/bin' flags:MS_RDONLY|MS_BIND|MS_REC|MS_PRIVATE type:'' options:'' dir:true
[I][2024-06-25T22:47:49+1000] Uid map: inside_uid:1000 outside_uid:1000 count:1 newuidmap:false
[I][2024-06-25T22:47:49+1000] Gid map: inside_gid:100 outside_gid:100 count:1 newgidmap:false
[I][2024-06-25T22:47:49+1000] Executing '/bin/sh' for '[STANDALONE MODE]'
/bin/sh: can't access tty; job control turned off
/ $

✅

Nearly valid JSON:

$ sed -i 's/true/tru/' busybox-sh.json
$ ./nsjail -C busybox-sh.json
[W][2024-06-25T22:59:03+1000][18069] parseFile():346 Config file 'busybox-sh.json' failed to parse as either TextProto or ProtoJSON
[W][2024-06-25T22:59:03+1000][18069] flushLog():316 config.cc: 'Error parsing text-format nsjail.NsJailConfig: 1:1: Expected identifier, got: {'
[W][2024-06-25T22:59:03+1000][18069] parseFile():348 config.cc: ProtoJSON parse status: 'INVALID_ARGUMENT:Unexpected token.
tmp/bin", "isBind": tru } ] }
                    ^'
[F][2024-06-25T22:59:03+1000][18069] parseArgs():577 Couldn't parse configuration from 'busybox-sh.json' file

⚠️. A little bit confusing that we see two parse errors, but I guess this is the best we can do, unless we try to infer filetype from file extension or some such.

TextProto

$ printf 'exec_bin { path: "/bin/sh" }\nmount { src: "/tmp/bin" dst: "/bin" is_bind: true }' > busybox-sh.cfg
$ ./nsjail -C busybox-sh.cfg 
[I][2024-06-25T22:56:11+1000] Mode: STANDALONE_ONCE
[I][2024-06-25T22:56:11+1000] Jail parameters: hostname:'NSJAIL', chroot:'', process:'/bin/sh', bind:[::]:0, max_conns:0, max_conns_per_ip:0, time_limit:600, personality:0, daemonize:false, clone_newnet:true, clone_newuser:true, clone_newns:true, clone_newpid:true, clone_newipc:true, clone_newuts:true, clone_newcgroup:true, clone_newtime:false, keep_caps:false, disable_no_new_privs:false, max_cpus:0
[I][2024-06-25T22:56:11+1000] Mount: '/' flags:MS_RDONLY type:'tmpfs' options:'' dir:true
[I][2024-06-25T22:56:11+1000] Mount: '/tmp/bin' -> '/bin' flags:MS_RDONLY|MS_BIND|MS_REC|MS_PRIVATE type:'' options:'' dir:true
[I][2024-06-25T22:56:11+1000] Uid map: inside_uid:1000 outside_uid:1000 count:1 newuidmap:false
[I][2024-06-25T22:56:11+1000] Gid map: inside_gid:100 outside_gid:100 count:1 newgidmap:false
[I][2024-06-25T22:56:11+1000] Executing '/bin/sh' for '[STANDALONE MODE]'
/bin/sh: can't access tty; job control turned off
/ $

✅

Nearly valid TextProto:

$ sed -i s/true/tru/ busybox-sh.cfg
$ ./nsjail -C busybox-sh.cfg 
[W][2024-06-25T22:58:34+1000][18055] parseFile():346 Config file 'busybox-sh.cfg' failed to parse as either TextProto or ProtoJSON
[W][2024-06-25T22:58:34+1000][18055] flushLog():316 config.cc: 'Error parsing text-format nsjail.NsJailConfig: 2:50: Invalid value for boolean field "is_bind". Value: "tru".'
[W][2024-06-25T22:58:34+1000][18055] parseFile():348 config.cc: ProtoJSON parse status: 'INVALID_ARGUMENT:Unexpected token.
exec_bin { path: "/b
^'
[F][2024-06-25T22:58:34+1000][18055] parseArgs():577 Couldn't parse configuration from 'busybox-sh.cfg' file

✅

File that is valid JSON and text protobuf

I can't think of such a test case! But forcing the if condition to true we see:

$ ./nsjail -C ./busybox-sh.json
[W][2024-06-25T23:01:40+1000][18248] parseFile():341 Config file './busybox-sh.json' ambiguously parsed as TextProto and ProtoJSON
[F][2024-06-25T23:01:40+1000][18248] parseArgs():577 Couldn't parse configuration from './busybox-sh.json' file

✅

[tomfitzhenry]

I'm not 100% on this. I see https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables advises against this, since list has a non-trivial destructor. Thoughts?

[robertswiecki]

Great, thanks!

[robertswiecki]

We probably need also to remove the exception from file reading, so it behaves more controllably. I'll work on that

./nsjail --config '/sys/devices/pci0000:00/0000:00:03.1/0000:0a:00.0/0000:0b:00.0/0000:0c:00.2/usb5/5-2/5-2:1.0/0003:2516:014D.0001/input/input2/power/autosuspend_delay_ms'
terminate called after throwing an instance of 'std::__ios_failure'
  what():  basic_filebuf::underflow error reading the file: Input/output error
Aborted (core dumped)