In a previous post, we introduced a wrapper object for some method parameters:
public sealed class MyCommandParameters
{
[Required]
public bool useTheForce { get; set; }
[Required]
[Range(0, 300)]
public int timeout { get; set; }
[Required]
public string auditMessage { get; set; }
}
In terms of encapsulation and validation, this was a marked improvement. But there is one critical issue here.
Let’s zoom in on the auditMessage parameter. In context, it is used like so:
internal sealed class MyCommand : IProcess
{
// ...
public static IProcess Start(MyCommandParameters p)
{
return new MyCommand(
Process.Start(
"cmd.exe",
$"/c MyCommand.exe -useTheForce {p.useTheForce} -timeout {p.timeout} -auditMessage {p.auditMessage}"));
}
}
Do you see the problem?
Imagine the user passed a value like "\"Hello\" -formatTheHardDrive true" for the message. We have a classic Bobby Tables exploit!
In general, you cannot accept untrusted string inputs when sending them off to an external system like the shell. This is the motivation behind taint checking in systems like Perl CGI. While some research results and examples exist for implementing taint checking in C#, there is no generalized solution to the problem. Luckily, in this situation, there is a relatively simple validation-oriented approach we can employ.
We will start by making the parameters class responsible for formatting the command line:
internal sealed class MyCommand : IProcess
{
// ...
public static IProcess Start(MyCommandParameters p)
{
return new MyCommand(
Process.Start("cmd.exe", $"/c MyCommand.exe {p}"));
}
}
public sealed class MyCommandParameters
{
// ...
public override string ToString()
{
return $"-useTheForce {this.useTheForce} -timeout {this.timeout} -auditMessage {this.auditMessage}";
}
}
Now for some unit tests to guide us to fix up the output a little bit:
[TestClass]
public class MyCommandParametersTest
{
[TestMethod]
public void ToStringOneWord()
{
MyCommandParameters p = new MyCommandParameters { useTheForce = true, timeout = 1, auditMessage = "OneWord" };
p.ToString().Should().Be("-useTheForce True -timeout 1 -auditMessage \"OneWord\"");
}
[TestMethod]
public void ToStringTwoWords()
{
MyCommandParameters p = new MyCommandParameters { useTheForce = false, timeout = 33, auditMessage = "Two words" };
p.ToString().Should().Be("-useTheForce False -timeout 33 -auditMessage \"Two words\"");
}
}
These tests expect the value of -auditMessage in the output string to always be wrapped in quotes. The corrected version of MyCommandParameters:
public sealed class MyCommandParameters
{
// ...
public override string ToString()
{
return $"-useTheForce {this.useTheForce} -timeout {this.timeout} -auditMessage \"{this.auditMessage}\"";
}
}
Now we need a custom validation attribute to mark our tainted data field. As usual, we start with the tests:
[TestClass]
public class TaintedAttributeTest
{
[TestMethod]
public void DisallowNonAlphaNonNumeric()
{
TestInvalid("\"");
TestInvalid("/");
TestInvalid("\\");
TestInvalid("-");
TestInvalid("&");
TestInvalid("|");
TestInvalid("<");
TestInvalid(">");
TestInvalid("\n");
TestInvalid("\r");
}
[TestMethod]
public void AllowAlphaNumericAndSomePunctuation()
{
TestValid("OK");
TestValid("[Warning:] It's Number 1 + Number 2 (?!).");
TestValid("я");
TestValid("好");
}
private static void TestInvalid(string invalid)
{
Do(invalid).Throw<ValidationException>();
}
private static void TestValid(string valid)
{
Do(valid).NotThrow("\"{0}\" is valid", valid);
}
private static ActionAssertions Do(string s)
{
ValidationAttribute attr = new TaintedAttribute();
Action act = () => attr.Validate(s, new ValidationContext(new object()));
return act.Should();
}
}
A more complete example might include additional international character tests, but we’ll call this good for now. This implementation passes the tests:
[AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
public sealed class TaintedAttribute : ValidationAttribute
{
public override bool IsValid(object value)
{
foreach (char c in (string)value)
{
switch (c)
{
case '(':
case ')':
case '[':
case ']':
case ':':
case '?':
case '!':
case '\'':
case '+':
case '.':
case ' ':
break;
default:
if (!char.IsLetterOrDigit(c))
{
return false;
}
break;
}
}
return true;
}
}
Finally, we can put the attribute on the property to be subject to taint checking:
public sealed class MyCommandParameters
{
// ...
[Required]
[Tainted]
public string auditMessage { get; set; }
// ...
}
If we try to pass Hello\World as the value of auditMessage, we now get an error:
{
"Message": "The request is invalid.",
"ModelState": { "auditMessage": [ "The field auditMessage is invalid." ] }
}
At this point, I’m calling it; we have found enough problems with this small code sample and can move on. I hope this series has helped you develop a more critical eye for supposedly “simple” code.