Use clone instead of fork on pvf

[jpserrat]

@mrcnski Done the change on the prepare worker, once the prepare worker part is good I'll do the same for the execute worker.

This is based on https://github.com/koute/polkavm/blob/11beebd06276ce9b84f335350138479e714f6caf/crates/polkavm/src/sandbox/linux.rs#L711.

TODO

• Add a check for this capability at startup
• Add prdoc mentioning the new Secure Validator Mode (optional) requirement.

Related

Closes #2162

[mrcnski]

Looks really good! This will make us even more secure. 😁

After this PR, I would suggest moving away from security-related tickets. A decision has come to replace wasmtime with polkavm fairly soon: #2461. Polkavm is already secured by default. I mean, it will take some time to switch as we need to do a migration, so it is good that we secured what we have already. But we can focus on other areas now. :)

[mrcnski]

Nice! Just a few suggestions, and then it should be good!

[mrcnski]

@jpserrat Do you want to take a shot at updating the Implementer's Guide?

[jpserrat]

@jpserrat Do you want to take a shot at updating the Implementer's Guide?

Do you mean, adding the clone change in there? Sure!

@mrcnski Just to let you know, I'll be on vacation this week and only going to be on the 12th, so it will take a while until I can go back to this.

I made the changes that you asked for and also used the File type instead of RawFd to use the write_all, with the nix implementation of read and write, the write operation was blocking and I realised that it was because of the buffer size. In the end I thought that it was easier to convert the fd to File instead of doing the same implementation of the write_all

[mrcnski]

Awesome, thank you @jpserrat! Thanks for letting me know about your vacation. There's no rush with this PR. :)

About File, do you think it's possible to define a wrapper over it, calling it something like PipeFd? Have it's impl only expose a few methods, like from_raw_fd, read, write. This would make things a bit clearer, and it's good to restrict the API surface area to operations relevant to us, to prevent potential misuse by people in the future changing this code.

[mrcnski]

Do you mean, adding the clone change in there? Sure!

Hmm. It might be too much of an an implementation detail to mention in the implementer's guide. Low-level details should stay in the code, e.g. module docs. In the impl guide we should just say that the jobs have some additional sandboxing measures that workers don't have. But, we can mention clone in the top-level docs of common/workers/security/mod.rs. And I would move the clone args to that file also, so we can re-use them for the exec worker later. :)

[mrcnski]

Awesome! It looks really good, now we just need to do the same for execute-worker. 🥳 Please just remember about this comment. We'll need to get the stack size for execution from the executor params, as mentioned in that comment.

[mrcnski]

pdeathsig seems not to work in clone, breaking some tests. I'm inclined to just remove it along with the associated tests. As pointed out on the PR, in the usual case the children will already exit on their own. In the malicious case, the pdeathsig can be easily unset, limiting its usefulness.

[alexggh]

Overall, looks good to me left you some comments.

[alexggh]

From man page:
Only a privileged process (CAP_SYS_ADMIN) can employ CLONE_NEWCGROUP

Is that something we want ?

Not sure if that is already a hard requirement for running polkadot but it feels like it shouldn't be.

[mrcnski]

Good catch! I was going to include the CLONE_NEWUSER clone flag, which gives us this capability. From unshare(2):

          As with the child process created by [clone(2)](https://man7.org/linux/man-pages/man2/clone.2.html) with the
          CLONE_NEWUSER flag, the caller obtains a full set of
          capabilities in the new namespace.

However, for reasons I don't understand, clone with this flag fails if we already passed the same flag to unshare here, when sandboxing the parent worker process. So I was going to parametrize this function over whether the unshare capability is present - I just forgot to do it. If it is present, then the parent worker process already has CAP_SYS_ADMIN. That's also whyclone_flags is a fn and not a const @alindima.

[mrcnski]

Okay, made this change. If for some reason we cannot unshare, we will need the CAP_SYS_ADMIN so I added the clone flag for that case. Hopefully there are no issues doing so in the wild, because admittedly local testing and CI have not caught all the issues in the past. However, this new capability is optional, so worst-case scenario someone sees a warning and reports it to us.

[tdimitrov]

Good job! I really enjoyed reviewing this PR.

[alindima]

this can be a constant

[s0me0ne-unkn0wn]

Wow, good job, it's very appreciated!
Left some comments on things that are doubtful to me. It's possible I'm just not getting the reasoning behind them, but right now, some of them look to me like they should be fixed. Please follow up in discussions.
Either way, nicely done!

[s0me0ne-unkn0wn]

Yes, that looks good now! Thank you!