Merge pull request #683 from rhansen/curl

curl execution improvements

Makefile.am

@@ -66,6 +66,7 @@ handwritten_tests = \
 	t/dnsexit2.pl \
 	t/get_ip_from_if.pl \
 	t/geturl_connectivity.pl \
+	t/geturl_response.pl \
 	t/group_hosts_by.pl \
 	t/interval_expired.pl \
 	t/is-and-extract-ipv4.pl \

configure.ac

@@ -36,7 +36,18 @@ AC_PROG_MKDIR_P
 AC_PATH_PROG([FIND], [find])
 AS_IF([test -z "${FIND}"], [AC_MSG_ERROR(['find' utility not found])])
 
-AC_PATH_PROG([CURL], [curl])
+AC_ARG_WITH([curl],
+  [AS_HELP_STRING([[--with-curl[=CURL]]], [use CURL as absolute path to curl executable])],
+  [],
+  [with_curl=yes])
+AS_CASE([${with_curl}],
+  [[yes]], [AC_PATH_PROG([CURL], [curl])],
+  [[no]], [CURL=],
+  [
+    AC_MSG_CHECKING([for curl])
+    CURL=${with_curl}
+    AC_MSG_RESULT([${CURL}])
+  ]);
 AS_IF([test -z "${CURL}"], [AC_MSG_ERROR([curl not found])])
 
 AX_WITH_PROG([PERL], perl)

ddclient.in

@@ -125,6 +125,7 @@ if ($program =~ /test/i) {
     $cachedir = '.';
     $savedir = 'URL';
 }
+our @curl = (subst_var('@CURL@', 'curl'));
 
 our $emailbody = '';
 my $last_emailbody = '';
@@ -2635,7 +2636,7 @@ sub curl_cmd {
     my @params = @_;
     my $tmpfile;
     my $tfh;
-    my $system_curl = quotemeta(subst_var('@CURL@', 'curl'));
+    my $curl = join(' ', @curl);
     my %curl_codes = (  ## Subset of error codes from https://curl.haxx.se/docs/manpage.html
          2 => "Failed to initialize. (Most likely a bug in ddclient, please open issue at https://github.com/ddclient/ddclient)",
          3 => "URL malformed. The syntax was not correct",
@@ -2653,11 +2654,11 @@ sub curl_cmd {
         67 => "The user name, password, or similar was not accepted and curl failed to log in.",
         77 => "Problem with reading the SSL CA cert (path? access rights?).",
         78 => "The resource referenced in the URL does not exist.",
-       127 => "$system_curl was not found",
+       127 => "$curl was not found",
     );
 
-    debug("CURL: %s", $system_curl);
-    fatal("curl not found") if ($system_curl eq '');
+    debug("CURL: %s", $curl);
+    fatal("curl not found") if ($curl[0] eq '');
     return '' if (scalar(@params) == 0); ## no parameters provided
 
     # Hard code to /tmp rather than use system TMPDIR to protect from malicious
@@ -2673,9 +2674,20 @@ sub curl_cmd {
         print($tfh @params);
     }
     close($tfh);
-    my $reply = qx{ $system_curl --config $tmpfile 2>/dev/null; };
+    # Use open's list form (as opposed to qx, backticks, or the scalar form of open) to avoid the
+    # shell and reduce the risk of a shell injection vulnerability.  ':raw' mode is used because
+    # HTTP is defined in terms of octets (bytes), not characters.  In raw mode, each byte from curl
+    # is mapped to a same-valued codepoint (byte value 0x78 becomes character U+0078, 0xff becomes
+    # U+00ff).  The caller is responsible for decoding the byte sequence if necessary.
+    open(my $cfh, '-|:raw', @curl, '--config', $tmpfile)
+        or fatal("failed to run curl ($curl): $!");
+    # According to <https://perldoc.perl.org/PerlIO#Alternatives-to-raw>, adding ':raw' to the open
+    # mode is buggy with Perl < v5.14.  Call binmode on the filehandle just in case.
+    binmode($cfh) or fatal("binmode failed: $!");
+    my $reply = do { local $/; <$cfh>; };
+    close($cfh);  # Closing $cfh waits for the process to exit and sets $?.
     if ((my $rc = $?>>8) != 0) {
-        warning("CURL error (%d) %s", $rc, $curl_codes{$rc} // "Unknown return code. Check $system_curl is installed and its manpage.");
+        warning("CURL error (%d) %s", $rc, $curl_codes{$rc} // "Unknown return code. Check $curl is installed and its manpage.");
     }
     return $reply;
 }

t/geturl_response.pl

@@ -0,0 +1,27 @@
+use Test::More;
+SKIP: { eval { require Test::Warnings; } or skip($@, 1); }
+eval { require 'ddclient'; } or BAIL_OUT($@);
+
+# Fake curl.  Use the printf utility, which can process escapes.  This allows Perl to drive the fake
+# curl with plain ASCII and get arbitrary bytes back, avoiding problems caused by any encoding that
+# might be done by Perl (e.g., "use open ':encoding(UTF-8)';").
+my @fakecurl = ('sh', '-c', 'printf %b "$1"', '--');
+
+my @test_cases = (
+    {
+        desc => 'binary body',
+        # Body is UTF-8 encoded ✨ (U+2728 Sparkles) followed by a 0xff byte (invalid UTF-8).
+        printf => join('\r\n', ('HTTP/1.1 200 OK', '', '\0342\0234\0250\0377')),
+        # The raw bytes should come through as equally valued codepoints.  They must not be decoded.
+        want => "HTTP/1.1 200 OK\n\n\xe2\x9c\xa8\xff",
+    },
+);
+
+for my $tc (@test_cases) {
+    @ddclient::curl = (@fakecurl, $tc->{printf});
+    $ddclient::curl if 0;  # suppress spurious warning "Name used only once: possible typo"
+    my $got = ddclient::geturl(url => 'http://ignored');
+    is($got, $tc->{want}, $tc->{desc});
+}
+
+done_testing();