Becoming a Perl bandit

Question

I've posted a few questions about natas a wargame. But I'm stuck at natas29 it has to do with Perl. I've never coded in Perl before.

Thus to help me beat the last few levels, I decided it is time to learn Perl. And what better way to learn it, then with wargames from overthewire. I made a small Perl program that beats the first few levels of bandit (in combination with some bash commands)

I'm specifically looking for answers that provide insights on how certain Perl operation could be abused, but any review is welcome.

#!/usr/bin/perl
use strict;
use warnings;

use Net::SSH::Perl;

my ($hostname, $username, $password, $port, $command) = ("bandit.labs.overthewire.org", "bandit0", "bandit0", "2220", "");

sub next_level {
    my ($user) = @_;
    $user =~ s/(\d+)\z/ $1 + 1 /e;
    return $user;
}

sub bandit_level {
    my ($pass, $user, $c) = @_;
    my $ssh = Net::SSH::Perl->new("$hostname", port=>$port, debug=>0);
    $ssh->login("$user","$pass");
    my ($stdout, $stderr, $exit) = $ssh->cmd("$c");
    my $new_pass = $stdout;
    return $new_pass;
}

sub read_file {
    my %hash;
    open CONFIG, "commands.txt" or die;
    while (my $line = <CONFIG>) {
        chomp $line;
        my ($lvl, $cmd) = split /\:/, $line;
        $hash{$lvl} = $cmd;
    }
    close CONFIG;
    return %hash;
}

my %hash = read_file;
my $bandit_count = scalar(keys %hash);

while ($bandit_count-- > 0)
{
    print "$username:$password\n";
    $command = $hash{$username};
    $password = bandit_level($password, $username, "$command");
    chomp $password;
    $password =~ s/\s//g;
    $username = next_level($username);
}

With commands.txt being this:

bandit0:cat readme
bandit1:cat ./-
bandit2:cat 'spaces in this filename'
bandit3:cat inhere/.hidden
bandit4:cat inhere/./-file07
bandit5:cat inhere/./maybehere07/.file2
bandit6:cat /var/lib/dpkg/info/bandit7.password
bandit7:cat data.txt | grep millionth | sed -e 's/^millionth//'
bandit8:cat data.txt | sort | uniq -u
bandit9:strings data.txt | grep '=' | cut -d '=' -f11 | tail -c 36
bandit10:base64 -d data.txt

Not yet another perl hacker ;)

Answer 1

This answer is going to talk about your style, and best practices. Some of it may be subjective. I'll go through the code line by line.

use strict;
use warnings;

Good job! Modern Perl programs should always start with strict and warnings. If you want more detailed information on what's wrong, you can add use diagnostics, but remember to remove it before you ship, because it is really heavy and just a debugging aid. You can also turn on use warnings 'all'.

---

my ($hostname, $username, $password, $port, $command) = ("bandit.labs.overthewire.org", "bandit0", "bandit0", "2220", "");

As you can see from the scrollbar, this line is long. For two or three variables, assigning them as a list is fine, but in this case, I would break them into five statements. It's way clearer, especially since $username and $password have the same value.

In addition, you do not need to put numbers into quotes, unless you need a leading zero (i.e. for a ZIP, ISBN or EAN code).

Some people prefer single quotes '' when there is no interpolation, but that really doesn't matter and is a question of personal preference. For the empty string, I prefer to use q{}, as that clear shows the intention of an empty string, over a forgotten string or a typo that made it to production, because of the pair delimiters.

my $hostname = 'bandit.labs.overthewire.org';
my $username = 'bandit0';
my $password = 'bandit0';
my $port     = 2220;
my $command  = q{};

---

$user =~ s/(\d+)\z/ $1 + 1 /e;

This line is fine, but I would like to point out the /x modifier, that allows you to use comments on the pattern part of the substitution. You can also change the delimiter, so this could (but in this case should probably not) become:

$user =~ s{
    (\d+) # grab lots of numbers
    \z    # at the end of the line
}{ $1 + 1 }ex;

---

my ($pass, $user, $c) = @_;

What is $c? I guess it is the command, but it's not as nicely verbose as your other variables. I do a lot of work with the Catalyst framework, and to me, $c is hardwired to the Catalyst context object. One-letter variable names are seldom the right choice. Always go for clarity.

---

my $ssh = Net::SSH::Perl->new("$hostname", port=>$port, debug=>0);

You do not need to quote variable names. New Perls will probably optimize this away, but it's ugly and might introduce bugs. You've done that a couple of times (though not consistently), and I'm not going to mention it again.

Also, your spacing here is inconsistent. You are using spaces around the equal (=), so why not do the same around the fat comma (=>)?

my $ssh = Net::SSH::Perl->new($hostname, port => $port, debug => 0);

my ($stdout, $stderr, $exit) = $ssh->cmd("$c");
my $new_pass = $stdout;
return $new_pass;

You only need the $stdout variable. The other two are discarded, so there's no reason to assign them at all. You can assign things to undef in a list context, and those parts of the list on the right hand side will be thrown away.

my (undef, $two, undef) = (1, 2, 3);

Since you only want the first value, there's no need to even do that. In list context the assignment will start with the right-most values, and all the later elements on the RHS will be discarded if there is nothing to assign them to on the LHS.

The assignment to $new_pass is not necessary because you just return that value. I know it has some kind of documentary value, but you can just name your variable like that in the first place. $stdout and $stderr only need to be named that if you need to distinguish them. But in this case, you don't care how Net::SSH::Perl got your password.

my ($new_pass) = $ssh->cmd($c);
return $new_pass;

However, you might want to check for errors somehow. Not sure what those commands return. You maybe want to keep $exit to look at it.

---

my %hash;

This is a really bad name. The sigil % already tells us it's a hash. This is like naming your child Child. The hash holds levels and commands. Why not name it %commands?

---

open CONFIG, "commands.txt" or die;
while (my $line = <CONFIG>) {
    # ...
}

This is a big one. Don't use glob filehandles. The CONFIG thing is global. It's the same everywhere in your program. Even in modules from CPAN. If you were to call some function while that CONFIG filehandle is opened, and that module were also using one with the same name, it would interfere with yours. Instead, use a lexical filehandle.

This has the added benefit that you don't have to use close explicitly in this case, as the lexical variable will go out of scope at the end of the function call anyway, and thus the filehandle will implicitly be closed.

You also should use the three-argument form of open. Your two arg without a mode implies reading, which is fine with this fixed string filename here. But what if the name comes from somewhere else? What if it's |rm -rf /? Ooops. You just opened a pipe to a command that deleted your harddrive. Bad luck. Instead, always pass the mode as the second argument, and that kind of thing cannot happen.

You do have or die, which is good, but your error will be Died at .... That's not very helpful. Instead, at least include the error that was returned by open, which is in $!. Putting the filename in that error message is helpful too.

open my $fh, '<', 'commands.txt' or die "Can't open commands.txt: $!";
while (my $line = <$fh>) { ... }

my ($lvl, $cmd) = split /\:/, $line;

The colon : is not a meta character in regex. You don't need to escape it.

---

 my $bandit_count = scalar(keys %hash);

You don't need the parentheses here. In fact, you hardly ever need them in Perl, which makes the code read more like English.

And there's the %hash again... Actually it's another %hash. Using the same variable name in an outer and inner scope it not the best idea, as it's ambiguous.

my $bandit_count = scalar keys %hash;

---

while ($bandit_count-- > 0)
{

Your style of placing the curlies is inconsistent. Above you had them on the same line as the while loop, now they are in the same line. Decide for one.

That loop could be implemented in a lot of ways. Since you don't care about the loop variable, the way you picked is not very Perlish at all. In fact, you're prone to getting an off-by-one error (or two?). The $foo-- decrement operator returns the current value of the variable, and then reduces it by one. Is that really what you need here?

It would be easier to get rid of the $bandid_count variable completely, and simply use a range with a for or foreach loop. Both are the same syntactically. Some people prefer the short for, but I think for the non-C style loop the foreach is easier to read.

foreach (1 .. keys %commands) {
     # ...    
}

You can leave out the [scalar][9] here as well, as the .. range operator already puts keys into scalar context.

---

print "$username:$password\n";

Instead of print and the newline, you can use say, which is shorter and includes the trailing line break. It's included in Perls starting from version 5.10 and needs to be turned on with use feature 'say' or a feature bundle.

---

In total, this code is pretty solid. If you really just started learning, you've done a great job. But then you are probably an experienced developer already, which is worth a lot.

If you wanted to replace the whole read_file sub with something more Perlish, you could do this:

sub read_file {
    return do {
        open my $fh, '<', 'commands.txt'
            or die "Can't open commands.txt: $!";
        map { chomp; split /:/ } <$fh>;
    };
}

The code makes use of a couple of features. The do block groups a number of statements into one. It's useful to know that every block's return value is always the return value of the last statement. That's also true for subs, so if there is no explicit return, chances are something will go wrong at some point.

map is like a foreach loop, and you probably know it from other languages that implement the map and reduce pattern. In the block, the topic variable $_ is used to iterate the list that comes out of the <$fh> diamond operator, which will just return the whole file as a list of lines in list context. Both chomp and split work on $_ implicitly if no arguments are passed, which makes this really concise.

---

If I may suggest another resource, the book Beginning Perl by Curtis "Ovid" Poe is great for developers that want to switch to Perl, as it has a nice walk-through, but does not explain everything down to how programming works, like Learning Perl would. To get more of the new stuff that's in the language, take a look at chromatic's Modern Perl, which is available for free online. Finally, there are some nice tricks in Effective Perl by Joseph N. Hall, Joshua A. McAdams and brian d foy. Several of the authors of these books are frequent users on Stack Overflow too.

Finally, something that might make your life a bit easier as these challenges get harder is Object::Remote. It allows you to execute Perl code on remote systems via SSH, without installing any modules there. You can use everything you have locally as long as it's compatible with the Perl that's there remotely, down to 5.8. Here is a great talk about it by the author from the German Perl Workshop 2014.