Re: Why `system` is safer with lists

[mlists@xxxxxxxxx]

On 4/10/24 22:37, jrmu wrote:
> Thanks.
>
> Can you also submit for me a diff/patch for vpsnow? I'll go ahead and
> try to merge it once I get gotwebd back in working order.

Alright, there should be a diff attached to this mail.

I'd recommend still testing it. I did test it myself, but you never know.

Some other notes and nitpicks:

- Even with system() or exec() being provided their arguments in list
  form, you still have to be careful about things like `su -l username
  -c "..."` for the same reason. I adjusted this script to not need su
  anymore, but thought I'd mention it.

- It would be better to hash the password using Perl if possible, rather
  than calling encrypt. The problem with calling encrypt and providing
  the password as an argument is this: anyone that has access to the
  system, even unprivileged access, can set up a script that watches the
  process tables. They can use this to grab temporary passwords during
  your account setup process.

- According to `perldoc -tf rand`, rand() is not cryptographically secure
  and shouldn't be used in security sensitive situations. I noticed
  you all use it to generate temporary passwords. I thought I'd make a
  note of it, though I'm not sure how much of an issue this actually
  is in reality. I think there's likely much lower hanging fruit for
  attackers to focus on than potential cryptographic flaws in your
  password generation scheme.

- I like to add `use autodie;` to scripts, it makes life easier. Then
  you no longer have to write `or die "..."` for various things like
  open. If you're alright with using IPC::System::Simple (a third party
  module for making system calls), you can do `use autodie qw(:all);`
  instead, and the autodie pragma will apply to system() and exec()
  as well. Error handling for IPC in Perl tends to be a pain in my
  experience, and failing early is safer. I noticed in testing that when
  I had bad commands, the script would keep running anyway.

These are just offhand remarks. Hope that helps. :)

diff /home/user/src/vpsnow
commit - a4f32e6b5c875752bffe3087a8964c833036f167
path + /home/user/src/vpsnow
blob - f3831d91f8f8c401aa62b0f342ce2bd04c8786ea
file + install.pl
--- install.pl
+++ install.pl
@@ -14,13 +14,13 @@ my $ipv4path = "/home/username/ipv4s";
 my $isopath = "/home/iso/install73.iso";
 my @ipv4s;
 if (!(-s "$ipv4path")) {
-        print "No IPv4 addresses in $ipv4path!\n";
+	print "No IPv4 addresses in $ipv4path!\n";
 	die;
 } else {
-        @ipv4s = readarray($ipv4path);
+	@ipv4s = readarray($ipv4path);
 }
 
-`doas chmod -R g+w $zonedir`;
+system qw(doas chmod -R g+w), $zonedir;
 
 # Read from filename and return array of lines without trailing newlines
 sub readarray {
@@ -91,7 +91,7 @@ sub setdns {
 	# trailing newline necessary
 	writefile("$filename.bak", join("\n", @lines)."\n");
 	copy "$filename.bak", $filename;
-	if (system("doas -u _nsd nsd-control reload")) {
+	if (system(qw(doas -u _nsd nsd-control reload))) {
 		return 0;
 	} else {
 		return 1;
@@ -115,14 +115,15 @@ sub nextdns {
 }
 
 sub createshell {
-        my ($username, $password) = @_;
+	my ($username, $password) = @_;
 	print "Username: $username\n";
 	print "Password: $password\n";
-        system "doas groupadd $username";
-        system "doas adduser -batch $username $username $username `encrypt $password`";
-        system "doas usermod -G vmdusers $username";
-        system "doas chmod -R o-rwx /home/$username";
-        system "doas su -l $username -c \"vmctl create -s 20G $username.qcow2\"";
+	chomp(my $hashed_password = `encrypt $password`);
+	system qw(doas groupadd), $username;
+	system qw(doas adduser -batch), $username, $username, $username, $hashed_password;
+	system qw(doas usermod -G vmdusers), $username;
+	system qw(doas chmod -R o-rwx), "/home/$username";
+	system qw(doas -u), $username, qw(vmctl create -s 20G), "/home/$username/$username.qcow2";
 	print "VM created for $username!\n";
 	my @vmconf = readarray($vmconf);
 	my $lladdr;
@@ -147,7 +148,7 @@ vm "$username" {
 }
 EOF
 	appendfile($vmconf, $block);
-	`doas vmctl reload`;
+	system qw(doas vmctl reload);
 }
 
 my $nargs = $#ARGV + 1;