Better nsjail - #53
Conversation
Migrating in-transit flag handling into this branch.
Locally composed image the improved handler works!
Monster0506
left a comment
Monster0506
left a comment
There was a problem hiding this comment.
Other than these few things, LGTM!
|
|
||
| if (requestedPath.startsWith(BINS_FOLDER)) { | ||
| // check executable binary files | ||
| if (await CheckFile(BINS_FOLDER, jail_bin)) continue; |
There was a problem hiding this comment.
This will always be true as CheckFile() always resolves to an object ({success, rc, message}), never false.So this continue fires, meaning ParseJailConfig never rejects a config that references a missing challenge binary or entrypoint. Destructure success here, like line 124 does: const { success, rc, message } = await CheckFile(NSJAIL_CONFS_FOLDER, nsjail_conf, false);
There was a problem hiding this comment.
Ah good catch! That's a simple fix, I probably repeated this in other places so I'll double check when I get to this.
|
|
||
| if (!MIN_PORT || !MAX_PORT || !BINS_FOLDER) { | ||
| console.error("[-] Missing values for MIN_PORT and MAX_PORT!") | ||
| if (MIN_PORT === MAX_PORT) { |
There was a problem hiding this comment.
If only one of the two env vars is actually missing (e.g. MIN_PORT=5000, MAX_PORT unset defaults to 0, "5000" !== 0) the check passes incorrectly. This is also duplicated in handler.js.
Also, it explicitly disallows 1 port, which I think is fine, not sure why we would only ever have 1 port.
There was a problem hiding this comment.
Yeah i was not super sure how to really validate this, I figured if we forget to set these they'd be the same like resolving as undefined. Ultimately I figured either the values match or there would be undefined === undefined.
There was a problem hiding this comment.
!MIN_PORT || !MAX_PORT
The previous check would do this validation fine: !MIN_PORT || !MAX_PORT
As !undefined is true, !0 is true, etc, and !"string" or !Non-zero-int is false.
| export async function GetUnusedPort(MIN, MAX) { | ||
| // Generate the port list | ||
| const ports = []; | ||
| for (let port = MIN; port <= MAX; port++) { |
There was a problem hiding this comment.
I think MIN and MAX are passed as strings to this function, meaning that something like "999" would be > "1000"
There was a problem hiding this comment.
Ill add a Number cast just to be safe, I didn't anticipate that.
| for (const f of jail_conf.files) { | ||
| const requestedPath = path.normalize(f); | ||
| if (requestedPath.startsWith("/lib")) { | ||
| // @todo - This might need expanded to support other additions |
There was a problem hiding this comment.
No changes requested here, just curious what else we might expand to here?
There was a problem hiding this comment.
I'm not sure what other files we'd want to link directly from the host other than like libc.so.6, not sure if we'd want anything from say /etc or any other locations.
There was a problem hiding this comment.
This might depend on challenge implementation, I know this was something I used with my ssh challenges
3 participants
Items regarding nsjail are better-worded in hopes to reduce confusion by admins. Jails can be widely configured through special json config files, there is a HANDLER.md that contains documentation for these nsjail config files. These configurations allow admins making challenges to set the main binary nsjail will execute along with defining a list of files the jail must contain to properly function.