{"id":5538,"date":"2018-10-14T13:00:22","date_gmt":"2018-10-14T13:00:22","guid":{"rendered":"http:\/\/writeasync.net\/?p=5538"},"modified":"2018-10-13T15:41:20","modified_gmt":"2018-10-13T15:41:20","slug":"find-the-issue-tainted-strings","status":"publish","type":"post","link":"http:\/\/writeasync.net\/?p=5538","title":{"rendered":"Find the issue: tainted strings"},"content":{"rendered":"

In a previous post<\/a>, we introduced a wrapper object for some method parameters:<\/p>\n

\r\n    public sealed class MyCommandParameters\r\n    {\r\n        [Required]\r\n        public bool useTheForce { get; set; }\r\n\r\n        [Required]\r\n        [Range(0, 300)]\r\n        public int timeout { get; set; }\r\n\r\n        [Required]\r\n        public string auditMessage { get; set; }\r\n    }\r\n<\/pre>\n
In terms of encapsulation and validation, this was a marked improvement. But there is one critical issue here.<\/p>\n
Let’s zoom in on the auditMessage<\/code> parameter. In context, it is used like so:<\/p>\n
\r\n    internal sealed class MyCommand : IProcess\r\n    {\r\n        \/\/ ...\r\n        public static IProcess Start(MyCommandParameters p)\r\n        {\r\n            return new MyCommand(\r\n                Process.Start(\r\n                    "cmd.exe",\r\n                    $"\/c MyCommand.exe -useTheForce {p.useTheForce} -timeout {p.timeout} -auditMessage {p.auditMessage}"));\r\n        }\r\n    }\r\n<\/pre>\n
Do you see the problem?<\/p>\n
Imagine the user passed a value like \"\\\"Hello\\\" -formatTheHardDrive true\"<\/code> for the message. We have a classic Bobby Tables<\/a> exploit!<\/p>\n
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<\/a> in systems like Perl CGI<\/a>. While some research results<\/a> and examples exist for implementing taint checking in C#<\/a>, there is no generalized solution to the problem. Luckily, in this situation, there is a relatively simple validation-oriented approach we can employ.<\/p>\n
We will start by making the parameters class responsible for formatting the command line:<\/p>\n
\r\n    internal sealed class MyCommand : IProcess\r\n    {\r\n        \/\/ ...\r\n        public static IProcess Start(MyCommandParameters p)\r\n        {\r\n            return new MyCommand(\r\n                Process.Start("cmd.exe", $"\/c MyCommand.exe {p}"));\r\n        }\r\n    }\r\n\r\n    public sealed class MyCommandParameters\r\n    {\r\n        \/\/ ...\r\n        public override string ToString()\r\n        {\r\n            return $"-useTheForce {this.useTheForce} -timeout {this.timeout} -auditMessage {this.auditMessage}";\r\n        }\r\n    }\r\n<\/pre>\n
Now for some unit tests to guide us to fix up the output a little bit:<\/p>\n
\r\n    [TestClass]\r\n    public class MyCommandParametersTest\r\n    {\r\n        [TestMethod]\r\n        public void ToStringOneWord()\r\n        {\r\n            MyCommandParameters p = new MyCommandParameters { useTheForce = true, timeout = 1, auditMessage = "OneWord" };\r\n\r\n            p.ToString().Should().Be("-useTheForce True -timeout 1 -auditMessage \\"OneWord\\"");\r\n        }\r\n\r\n        [TestMethod]\r\n        public void ToStringTwoWords()\r\n        {\r\n            MyCommandParameters p = new MyCommandParameters { useTheForce = false, timeout = 33, auditMessage = "Two words" };\r\n\r\n            p.ToString().Should().Be("-useTheForce False -timeout 33 -auditMessage \\"Two words\\"");\r\n        }\r\n    }\r\n<\/pre>\n
These tests expect the value of -auditMessage<\/code> in the output string to always be wrapped in quotes. The corrected version of MyCommandParameters<\/code>:<\/p>\n
\r\n    public sealed class MyCommandParameters\r\n    {\r\n        \/\/ ...\r\n        public override string ToString()\r\n        {\r\n            return $"-useTheForce {this.useTheForce} -timeout {this.timeout} -auditMessage \\"{this.auditMessage}\\"";\r\n        }\r\n    }\r\n<\/pre>\n
Now we need a custom validation attribute to mark our tainted data field. As usual, we start with the tests:<\/p>\n
\r\n    [TestClass]\r\n    public class TaintedAttributeTest\r\n    {\r\n        [TestMethod]\r\n        public void DisallowNonAlphaNonNumeric()\r\n        {\r\n            TestInvalid("\\"");\r\n            TestInvalid("\/");\r\n            TestInvalid("\\\\");\r\n            TestInvalid("-");\r\n            TestInvalid("&");\r\n            TestInvalid("|");\r\n            TestInvalid("<");\r\n            TestInvalid(">");\r\n            TestInvalid("\\n");\r\n            TestInvalid("\\r");\r\n        }\r\n\r\n        [TestMethod]\r\n        public void AllowAlphaNumericAndSomePunctuation()\r\n        {\r\n            TestValid("OK");\r\n            TestValid("[Warning:] It's Number 1 + Number 2 (?!).");\r\n            TestValid("\u044f");\r\n            TestValid("\u597d");\r\n        }\r\n\r\n        private static void TestInvalid(string invalid)\r\n        {\r\n            Do(invalid).Throw<ValidationException>();\r\n        }\r\n\r\n        private static void TestValid(string valid)\r\n        {\r\n            Do(valid).NotThrow("\\"{0}\\" is valid", valid);\r\n        }\r\n\r\n        private static ActionAssertions Do(string s)\r\n        {\r\n            ValidationAttribute attr = new TaintedAttribute();\r\n\r\n            Action act = () => attr.Validate(s, new ValidationContext(new object()));\r\n\r\n            return act.Should();\r\n        }\r\n    }\r\n<\/pre>\n
A more complete example might include additional international character tests, but we’ll call this good for now. This implementation passes the tests:<\/p>\n
\r\n    [AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]\r\n    public sealed class TaintedAttribute : ValidationAttribute\r\n    {\r\n        public override bool IsValid(object value)\r\n        {\r\n            foreach (char c in (string)value)\r\n            {\r\n                switch (c)\r\n                {\r\n                    case '(':\r\n                    case ')':\r\n                    case '[':\r\n                    case ']':\r\n                    case ':':\r\n                    case '?':\r\n                    case '!':\r\n                    case '\\'':\r\n                    case '+':\r\n                    case '.':\r\n                    case ' ':\r\n                        break;\r\n                    default:\r\n                        if (!char.IsLetterOrDigit(c))\r\n                        {\r\n                            return false;\r\n                        }\r\n\r\n                        break;\r\n                }\r\n            }\r\n\r\n            return true;\r\n        }\r\n    }\r\n<\/pre>\n
Finally, we can put the attribute on the property to be subject to taint checking:<\/p>\n
\r\n    public sealed class MyCommandParameters\r\n    {\r\n        \/\/ ...\r\n        [Required]\r\n        [Tainted]\r\n        public string auditMessage { get; set; }\r\n        \/\/ ...\r\n    }\r\n<\/pre>\n
If we try to pass Hello\\World<\/code> as the value of auditMessage<\/code>, we now get an error:<\/p>\n
\r\n{\r\n  "Message": "The request is invalid.",\r\n  "ModelState": { "auditMessage": [ "The field auditMessage is invalid." ] }\r\n}\r\n<\/pre>\n
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.<\/p>\n","protected":false},"excerpt":{"rendered":"
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… <\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[91,41],"tags":[],"class_list":["post-5538","post","type-post","status-publish","format-standard","hentry","category-design","category-tdd"],"_links":{"self":[{"href":"http:\/\/writeasync.net\/index.php?rest_route=\/wp\/v2\/posts\/5538","targetHints":{"allow":["GET"]}}],"collection":[{"href":"http:\/\/writeasync.net\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"http:\/\/writeasync.net\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"http:\/\/writeasync.net\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"http:\/\/writeasync.net\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=5538"}],"version-history":[{"count":3,"href":"http:\/\/writeasync.net\/index.php?rest_route=\/wp\/v2\/posts\/5538\/revisions"}],"predecessor-version":[{"id":5541,"href":"http:\/\/writeasync.net\/index.php?rest_route=\/wp\/v2\/posts\/5538\/revisions\/5541"}],"wp:attachment":[{"href":"http:\/\/writeasync.net\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=5538"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"http:\/\/writeasync.net\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=5538"},{"taxonomy":"post_tag","embeddable":true,"href":"http:\/\/writeasync.net\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=5538"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}