Injections: when data becomes a command

Code in:JavaC#PythonJavaScript

Problem

A thumbnail with a surprise

Buyers upload photos with their reviews. The shop’s tool makes a thumbnail of each one: it runs the image program (for example magick from ImageMagick) with the file’s name.

Here echo stands in for the real converter. It only prints its arguments, so the output shows exactly what reached the program:

static void makeThumbnail(String fileName) {
    String command = "echo resize " + fileName;
    run(new ProcessBuilder("sh", "-c", command));
}

static void run(ProcessBuilder builder) {
    try {
        Process process = builder.start();
        System.out.print(new String(process.getInputStream().readAllBytes()));
        process.waitFor();
    } catch (IOException | InterruptedException e) {
        throw new RuntimeException(e);
    }
}

makeThumbnail("shoes.jpg");
makeThumbnail("shoes.jpg; echo injected");

Output

resize shoes.jpg
resize shoes.jpg
injected

The first name is ordinary. The second is shoes.jpg; echo injected, and the output has three lines, not two. injected was printed by a command the buyer wrote into the file’s name. Here it is a harmless echo, but any command could stand in its place, and it runs with the rights of the shop’s server.

The idea

Text that ends up in a language

A command for the shell is a language. In it ; means “end of command, another one follows”. The code glued the buyer’s name into that language, and the characters in the name changed the meaning of the command. This is command injection.

You have seen the same with SQL. When a program glues ' OR '1'='1 into a query, the quote closes the text early and the condition becomes true for every row. The fix was a parameter: the query and the value go to the database separately. That is the lesson “Databases” in the language course and “SQL injection and parameters” in the SQL course, so we will not repeat it here.

Wherever text ends up in a language, some characters mean something:

Where the text ends up	Characters with meaning	How it stays data
an SQL query	', --, ;	a parameter
a command for the shell	;, &&, $( ), a space	an argument list, no shell
a file path	.., /, \, a path from the root	a canonical path and a check, or a number from a list
an HTML page	<, >, ", &	escaping (the next lesson)

The common idea: do not let data become part of the command. Hand it to the system separately, in a way the system never reads as a command.

See it

How the name becomes a command

Both cases from the lesson, before and after the fix. The code next to the scene changes with the step:

static void makeThumbnail(String fileName) {
    String command = "echo resize " + fileName;
    run(new ProcessBuilder("sh", "-c", command));
}

Step by step, in words

1. A buyer uploads a photo named “shoes.jpg; echo injected”. The shop’s tool makes a thumbnail from it.
2. The code glues the name into one string for the shell. The name is no longer data: it is part of a command.
3. To the shell, “;” ends a command. The string becomes two commands, and the buyer wrote the second. Here it only prints “injected”; it could be any command.
4. The fix: no shell. The program gets a list of arguments, and the whole name is one argument. Here “;” is an ordinary character in it.
5. The invoice download joins the name from the address to the invoices/ folder. The name is “../settings.txt”, and “..” means “one folder up”.
6. The path climbs out of invoices/ and the file read is settings.txt, next to it. The name chose a different file.
7. The fix: first work out where the path really ends (the canonical path), then check it is still inside invoices/. settings.txt is not: refused.

Try it

Find the line

The shop sends a receipt: it makes a PDF and sends it to the buyer’s email. Which line hands text from the buyer to the shell?

Pick the line with the bug.

1static void sendReceipt(int orderId, String email) {2 String pdf = "receipt-" + orderId + ".pdf";3 run(new ProcessBuilder("echo", "render", pdf));4 run(new ProcessBuilder("echo", "attach", pdf));5 run(new ProcessBuilder("sh", "-c", "echo send to " + email));6}

Check

The idea

An argument list instead of a string

The fix is not to “clean” the name. It is to have no shell that reads it. The program is started with an argument list: the first item is the program, and every item after it reaches the program whole, whatever characters it contains.

static void makeThumbnail(String fileName) {
    run(new ProcessBuilder("echo", "resize", fileName));
}

static void run(ProcessBuilder builder) {
    try {
        Process process = builder.start();
        System.out.print(new String(process.getInputStream().readAllBytes()));
        process.waitFor();
    } catch (IOException | InterruptedException e) {
        throw new RuntimeException(e);
    }
}

makeThumbnail("shoes.jpg");
makeThumbnail("shoes.jpg; echo injected");

Output

resize shoes.jpg
resize shoes.jpg; echo injected

The second name reached the converter as one argument, with the ; inside. There is no second command.

new ProcessBuilder("echo", "resize", fileName) starts echo directly, with no shell. Every string is a separate argument.

More precisely, if you are curious

• Best of all, do not start an external program at all. There are libraries for images: ImageIO from the standard library, or Thumbnailator. Then there is no command for text to end up in.
• Keep your own names. Save the uploaded file under a name the program makes up (a number or a UUID), and keep the buyer’s name as data in the database. Then only your name reaches the command and the path.
• An argument that starts with -. Without a shell ; is harmless, but the program may still read a name like -x as one of its options. That is why many programs accept -- (“end of options”) before the file names, or you put ./ in front of the name.
• Runtime.exec(String) does not start a shell: it splits the string at spaces. So ; does not make a second command there, but a space in the name splits it into two arguments. Do not rely on that: ProcessBuilder with a list says clearly what an argument is.

Problem

An invoice that is not an invoice

A buyer downloads an invoice from /invoices/1041.txt. The server takes the name from the address and joins it to the invoice folder:

static String download(String name) throws IOException {
    Path file = invoices.resolve(name);
    return Files.readString(file);
}

System.out.println(download("1041.txt"));
System.out.println(download("../settings.txt"));

Output

Invoice 1041: 59 €
payment_api_key=demo-not-real

The second name is ../settings.txt. In a path .. means “one folder up”. The path left invoices/, and the server read settings.txt, which sits next to it, with the payment key.

This is path traversal: the name in the request picks a file outside the folder the code had in mind. The program here makes its own temporary folder and reads only inside it; on a real server .. can climb many levels up.

The idea

A canonical path, then a check

The fix has two steps:

1. Work out where the path really ends. That is the canonical path: complete, with no . and ...
2. Check that the canonical path is still inside the invoice folder. If it is not, refuse.

static String download(String name) throws IOException {
    Path file = invoices.resolve(name).normalize();
    if (!file.startsWith(invoices)) {
        return "Refused: outside the invoice folder";
    }
    return Files.readString(file);
}

System.out.println(download("1041.txt"));
System.out.println(download("../settings.txt"));

Output

Invoice 1041: 59 €
Refused: outside the invoice folder

normalize() removes . and .., and Path.startsWith compares the parts of the path, not the text: invoices-old is not inside invoices.

The check does not look for dangerous characters. It asks where the path ends, so it does not depend on what you thought of.

Safer still is for the name not to be a path at all. The address carries an invoice number, and the server looks the file up in its own table: a number that is not there is refused. That is the exercise below.

More precisely, if you are curious

• Symbolic links. normalize() works only on the text of the path. If the folder may contain a symbolic link pointing outside, use toRealPath() (it needs the file to exist).
• A path from the root. invoices.resolve("/etc/hosts") returns /etc/hosts: the absolute path replaces the folder. The check catches it.
• Archives. The file names inside a zip archive are text from outside too. Unpacking must do the same check for every file (the trap has a name: zip slip).
• The setup. The program makes a temporary folder, writes the two invoices and the settings file into it, and deletes it at the end. Here is how it starts:

static Path invoices;

Path shop = Files.createTempDirectory("shop");
invoices = Files.createDirectory(shop.resolve("invoices"));
Files.writeString(invoices.resolve("1041.txt"), "Invoice 1041: 59 €");
Files.writeString(invoices.resolve("1042.txt"), "Invoice 1042: 118 €");
Files.writeString(shop.resolve("settings.txt"), "payment_api_key=demo-not-real");

Try it

The canonical path

Finish the line that works out where the path ends:

static String download(String name) throws IOException {
    Path file = invoices.resolve(name).();
    if (!file.startsWith(invoices)) {
        return "Refused: outside the invoice folder";
    }
    return Files.readString(file);
}

Check

Try it

Three names

What does the fixed download print for these three names?

System.out.println(download("1042.txt"));
System.out.println(download("../invoices/1042.txt"));
System.out.println(download("../settings.txt"));

Answer

Invoice 1042: 118 €
Refused: outside the invoice folder
Refused: outside the invoice folder

Invoice 1042: 118 €
Invoice 1042: 118 €
Refused: outside the invoice folder

Invoice 1042: 118 €
Invoice 1042: 118 €
payment_api_key=demo-not-real

Check

Exercise

The invoice by number

Solve it in your own editor.

By number, not by name. Write downloadById(id). The address carries only an invoice number. A table in the code says which number is which file (1041 → 1041.txt, 1042 → 1042.txt). A number that is not in the table is refused, and the file name never comes from the request. Try it with 1041, 1042, ../settings.txt and 1043:

Expected

Invoice 1041: 59 €
Invoice 1042: 118 €
Refused: no such invoice
Refused: no such invoice

More tasks

• Someone else’s invoice. The number 1042 is in the table, but the invoice belongs to another buyer. What else must the server check before returning it?
• The photo’s name. Change the thumbnail tool so that it saves the uploaded file under a name the program makes up (the review’s number and .jpg), and keeps the buyer’s name only as text.

Test

Check yourself

1. What do SQL injection, command injection and path traversal have in common?

Text from the user ends up in a language (SQL, the shell, a path) and its characters change the meaning of the commandAll three can only happen on websitesAll three are fixed by making the input shorter

2. The thumbnail tool starts the converter with an argument list, and the photo’s name is the last argument. What happens to a “;” in the name?

Nothing special: there is no shell to read it, so it is an ordinary character in the nameIt runs a second commandThe program crashes

3. The fixed download works out the canonical path and checks it is inside invoices/. What does it return for “../invoices/1042.txt”?

An error: no such fileInvoice 1042: the path goes out and comes back into invoices/, and the check looks at where it endsRefused: the name contains ..

4. Why is removing “..” and “;” from the input not the fix?

Because buyers have a right to such namesBecause it is slowThat is a deny-list: it lets through everything you did not think of. The safe way is to give the text no meaning (arguments, parameters) or to check the final result

5. A buyer uploads a photo. What is the safest way to save it on disk?

Under a name the program makes up (a number or a UUID), keeping the buyer’s name only as data in the databaseUnder the buyer’s name, after removing the spacesUnder the buyer’s name in a separate folder

Check

Start the lesson over

Related lessons

• Builds on: Lists · HashMap and HashSet · Databases · Untrusted input
• Continues in: XSS: someone else’s script in your page
• Tasks on this topic: Security for developers