Merge branch 'master' into debian

9 files changed, 74 insertions, 32 deletions

diff --git a/Changelog b/Changelog
index 59d5153..0674c4a 100644
--- a/Changelog
+++ b/Changelog
@@ -4,7 +4,7 @@ lacme (0.3) upstream;
     lacme-certs.conf.d"), import the default section of files read earlier.
   + new-cert: create certificate files atomically.
   + webserver: allow listening to multiple addresses (useful when
-    dual-stack IPv4/IPv6 is not supported).  Listen to a UNIX-domain
+    dual IPv4/IPv6 stack is not supported).  Listen to a UNIX-domain
     socket by default </var/run/lacme.socket>.
   + webserver: don't install temporary iptables by default.  Hosts
     without a public HTTP daemon listening on port 80 need to set the
@@ -12,7 +12,7 @@ lacme (0.3) upstream;
     'iptables' option to Yes.
   + Change 'min-days' default from 10 to 21, to avoid expiration notices
     from Let's Encrypt when auto-renewal is done by a cronjob.
-  + Provide nginx configuration snippet.
+  + Provide nginx and apache2 configuration snippets.
   - Ensure lacme's config file descriptor is not passed to the accountd
     or webserver components.
   - new-cert: sort section names if not passed explicitely.
@@ -21,6 +21,15 @@ lacme (0.3) upstream;
   - new-cert: mark the basicConstraints (CA:FALSE) and keyUsage x509v3
     extensions as critical in the CSR, following upstream fix of
     Boulder's issue #565.
+  - webserver: refuse to follow symlink when serving ACME challenge
+    responses.  When dropping privileges to a dedicated UID
+    (recommended) only the ACME client could write to its current
+    directory anyway, so following symlinks was not a serious
+    vulnerability.
+  - lacme(1), lacme-accountd(1): fix version number shown with
+    --version.
+  - client: remove potential race when creating ACME challenge response
+    files.
 
  -- Guilhem Moulin <guilhem@guilhem.org>  Sun, 19 Feb 2017 13:08:41 +0100
 
diff --git a/README b/README
index ea8cc66..5e17a8d 100644
--- a/README
+++ b/README
@@ -22,14 +22,12 @@ own executable:
     of the account key manager to the ACME client: data signatures are
     requested by writing the data to be signed to the socket.
 
-  * For certificate issuances, an optional webserver, which is spawned
-    by the "master" process when no service is listening on the HTTP
-    port.  (The only challenge type currently supported is "http-01",
-    which requires a webserver to answer challenges.)  That webserver
-    only processes GET and HEAD requests under the
-    "/.well-known/acme-challenge/" URI.  By default some iptables(8)
-    rules are automatically installed to open the HTTP port, and removed
-    afterwards.
+  * For certificate issuances, an optional webserver which is spawned by
+    the "master".  (The only challenge type currently supported is
+    "http-01", which requires a webserver to answer challenges.)  That
+    webserver only processes GET and HEAD requests under the
+    "/.well-known/acme-challenge/" URI.  Moreover temporary iptables(8)
+    rules can be automatically installed to open the HTTP port.
 
 Consult the manuals for more information.
 
diff --git a/client b/client
index cd94ed8..333ae3b 100755
--- a/client
+++ b/client
@@ -44,10 +44,13 @@ use warnings;
 
 my $PROTOCOL_VERSION = 1;
 
-use LWP::UserAgent ();
+use Errno 'EEXIST';
+use Fcntl qw/O_CREAT O_EXCL O_WRONLY/;
+use Digest::SHA qw/sha256 sha256_hex/;
 use MIME::Base64 qw/encode_base64 encode_base64url/;
+
+use LWP::UserAgent ();
 use JSON ();
-use Digest::SHA qw/sha256 sha256_hex/;
 
 use Config::Tiny ();
 
@@ -266,18 +269,19 @@ elsif ($COMMAND eq 'new-cert') {
                                @{request_json_decode($r)->{challenges} // []};
         die "Missing 'http-01' challenge in server response" unless defined $challenge;
         die "Invalid challenge token ".($challenge->{token} // '')."\n"
+            # ensure we don't write outside the cwd
             unless ($challenge->{token} // '') =~ /\A[A-Za-z0-9_\-]+\z/;
 
         my $keyAuthorization = $challenge->{token}.'.'.$JWK_thumbprint;
 
         # serve $keyAuthorization at http://$domain/.well-known/acme-challenge/$challenge->{token}
-        if (-e $challenge->{token}) {
-            print STDERR "WARNING: File exists: $challenge->{token}\n";
-        }
-        else {
-            open my $fh, '>', $challenge->{token} or die "Can't open $challenge->{token}: $!";
+        if (sysopen(my $fh, $challenge->{token}, O_CREAT|O_EXCL|O_WRONLY, 0644)) {
             $fh->print($keyAuthorization);
             $fh->close() or die "Can't close: $!";
+        } elsif ($! == EEXIST) {
+            print STDERR "WARNING: File exists: $challenge->{token}\n";
+        } else {
+            die "Can't open $challenge->{token}: $!";
         }
 
         $r = acme($challenge->{uri}, {
diff --git a/config/apache2.conf b/config/apache2.conf
new file mode 100644
index 0000000..20927fa
--- /dev/null
+++ b/config/apache2.conf
@@ -0,0 +1,12 @@
+# Use Apache2 to serve ACME requests by passing them over to a
+# locally-bound lacme webserver component.
+#
+# This file needs to be sourced to the server directives (at least the
+# non-ssl one) of each virtual host requiring authorization.
+
+<Location /.well-known/acme-challenge/>
+  ProxyPass unix:///var/run/lacme.socket|http://127.0.0.1/.well-known/acme-challenge/
+  Order allow,deny
+  Allow from all
+</Location>
+
diff --git a/config/nginx.conf b/config/nginx.conf
index f842c12..6753ff9 100644
--- a/config/nginx.conf
+++ b/config/nginx.conf
@@ -1,10 +1,10 @@
-# Let nginx serve ACME requests directly, or pass them to lacme's
-# webserver component.
+# Use Nginx to serve ACME requests; either directly, or by passing them
+# over to a locally-bound lacme webserver component.
 #
 # This file needs to be sourced to the server directives (at least the
 # non-ssl one) of each virtual host requiring authorization.
 
-location /.well-known/acme-challenge/ {
+location ^~ /.well-known/acme-challenge/ {
     # Pass ACME requests to lacme's webserver component
     proxy_pass http://unix:/var/run/lacme.socket;
 
diff --git a/lacme b/lacme
index d7a416e..1065e67 100755
--- a/lacme
+++ b/lacme
@@ -21,10 +21,10 @@
 use strict;
 use warnings;
 
-our $VERSION = '0.0.1';
+our $VERSION = '0.3';
 my $NAME = 'lacme';
 
-use Errno qw/EADDRINUSE EINTR/;
+use Errno 'EINTR';
 use Fcntl qw/F_GETFD F_SETFD FD_CLOEXEC SEEK_SET/;
 use File::Temp ();
 use Getopt::Long qw/:config posix_default no_ignore_case gnu_getopt auto_version/;
diff --git a/lacme-accountd b/lacme-accountd
index 00d6ccd..547af59 100755
--- a/lacme-accountd
+++ b/lacme-accountd
@@ -22,7 +22,7 @@
 use strict;
 use warnings;
 
-our $VERSION = '0.0.1';
+our $VERSION = '0.3';
 my $PROTOCOL_VERSION = 1;
 my $NAME = 'lacme-accountd';
 
diff --git a/lacme.md b/lacme.md
index 4146515..0f6f3ee 100644
--- a/lacme.md
+++ b/lacme.md
@@ -223,7 +223,8 @@ of [ACME] commands and dialogues with the remote [ACME] server).
 `[webserver]` section
 ---------------------
 
-This section is used for configuring the [ACME] webserver.
+This section is used to configure how [ACME] challenge responses are
+served during certificate issuance.
 
 *listen*
 
@@ -233,6 +234,16 @@ This section is used for configuring the [ACME] webserver.
     absolute path of a UNIX-domain socket (created with mode `0666`).
     Default: `/var/run/lacme.socket`.
 
+    **Note**: The default value is only suitable when an external HTTP
+    daemon is publicly reachable and passes all ACME challenge requests
+    to the webserver component through the UNIX-domain socket
+    `/var/run/lacme.socket` (for instance using the provided
+    `/etc/lacme/apache2.conf` or `/etc/lacme/nginx.conf` configuration
+    snippets for each virtual host requiring authorization).  If there
+    is no HTTP daemon bound to port 80 one needs to set *listen* to
+    `[::]` (or `0.0.0.0 [::]` when dual IPv4/IPv6 stack is disabled or
+    unavailable), and possibly also set *iptables* to `Yes`.
+
 *challenge-directory*
 
 :   Specify a non-existent directory under which an external HTTP daemon
@@ -258,7 +269,9 @@ This section is used for configuring the [ACME] webserver.
 
 *command*
 
-:   Path to the [ACME] webserver executable.
+:   Path to the [ACME] webserver executable.  A separate process is
+    spawned for each address to *listen* on.  (In particular no
+    webserver process is forked when the *listen* option is empty.)
     Default: `/usr/lib/lacme/webserver`.
 
 *iptables*
diff --git a/webserver b/webserver
index 7914762..90be70c 100755
--- a/webserver
+++ b/webserver
@@ -37,7 +37,8 @@ use warnings;
 # as "www-data:www-data"; bind(2)'ing to a privileged port such as 80 is
 # not a problem since FD can be bound as root prior to the execve(2).
 
-use Errno 'EINTR';
+use Errno qw/EINTR ENOENT/;
+use Fcntl qw/O_NOFOLLOW O_RDONLY/;
 use Socket qw/AF_UNIX AF_INET AF_INET6/;
 
 # Untaint and fdopen(3) the listening socket
@@ -91,18 +92,23 @@ while (1) {
     while (defined (my $h = $conn->getline())) { last if $h eq "\r\n" };
 
     my ($status_line, $content_type, $content);
-    if ($req =~ /\A\Q$ROOT\E\/([A-Za-z0-9_\-]+)\z/ and -f $1) {
-        if (open my $fh, '<', $1) { # only open files in the cwd
+    if ($req =~ /\A\Q$ROOT\E\/([A-Za-z0-9_\-]+)\z/) {
+        # only serve base64-encoded filenames (tokens) in the cwd
+        # XXX stat(2) followed by open(2) is racy; open(2) would hang if
+        # an attacker manages to replace a regular file with a FIFO
+        # between the syscalls, leading to denial of service; however
+        # there shouldn't be any risk of information disclosure as we're
+        # not following symlinks (and the cwd is not owned by us)
+        if (-f $1 and sysopen(my $fh, $1, O_NOFOLLOW|O_RDONLY)) {
             ($status_line, $content_type) = ('200 OK', 'application/jose+json');
             $content = do { local $/ = undef; $fh->getline() };
             $fh->close() or die "close: $!";
-        }
-        else {
-            $status_line = '403 Forbidden';
+        } elsif ($! == ENOENT) { # errno from either stat(2) or open(2)
+            $status_line = '404 Not Found';
         }
     }
 
-    $conn->print( "HTTP/$proto ", ($status_line // '404 Not Found'), "\r\n" );
+    $conn->print( "HTTP/$proto ", ($status_line // '403 Forbidden'), "\r\n" );
     $conn->print( "Content-Type: $content_type\r\n" ) if defined $content_type;
     $conn->print( "Content-Length: ".length($content)."\r\n" ) if defined $content;
     $conn->print( "Connection: close\r\n" );