diff mbox series

[bug#47282,v2,12/13] gnu: Add llhttp-bootstrap.

Message ID 20210330052743.575-12-samplet@ngyro.com
State Accepted
Headers show
Series [bug#47282,v2,01/13] build-system: Rewrite node build system. | expand

Checks

Context Check Description
cbaines/submitting builds success
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Timothy Sample March 30, 2021, 5:27 a.m. UTC 
From: Jelle Licht <jlicht@fsfe.org>

* gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/node.scm (llhttp-bootstrap): New variable.
---
 gnu/local.mk                                  |   1 +
 gnu/packages/node.scm                         |  70 ++++++++++++
 .../llhttp-bootstrap-CVE-2020-8287.patch      | 100 ++++++++++++++++++
 3 files changed, 171 insertions(+)
 create mode 100644 gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch

Comments

Efraim Flashner March 30, 2021, 6:59 a.m. UTC | #1 
On Tue, Mar 30, 2021 at 01:27:42AM -0400, Timothy Sample wrote:
> From: Jelle Licht <jlicht@fsfe.org>
> 
> * gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/packages/node.scm (llhttp-bootstrap): New variable.
> ---
>  gnu/local.mk                                  |   1 +
>  gnu/packages/node.scm                         |  70 ++++++++++++
>  .../llhttp-bootstrap-CVE-2020-8287.patch      | 100 ++++++++++++++++++
>  3 files changed, 171 insertions(+)
>  create mode 100644 gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch
> 
> diff --git a/gnu/local.mk b/gnu/local.mk
> index 52a021c2a3..5959a563d1 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -1366,6 +1366,7 @@ dist_patch_DATA =						\
>    %D%/packages/patches/linux-pam-no-setfsuid.patch		\
>    %D%/packages/patches/lirc-localstatedir.patch			\
>    %D%/packages/patches/lirc-reproducible-build.patch		\
> +  %D%/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch	\
>    %D%/packages/patches/llvm-3.5-fix-clang-build-with-gcc5.patch	\
>    %D%/packages/patches/llvm-9-fix-bitcast-miscompilation.patch	\
>    %D%/packages/patches/llvm-9-fix-lpad-miscompilation.patch	\
> diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
> index 5336012e43..45e5f8feca 100644
> --- a/gnu/packages/node.scm
> +++ b/gnu/packages/node.scm
> @@ -510,6 +510,76 @@ Node.js and web browsers.")
>  parser definition into a C output.")
>      (license license:expat)))
>  
> +(define-public llhttp-bootstrap
> +  (package
> +    (name "llhttp")
> +    (version "2.1.3")
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/nodejs/llhttp.git")
> +                    (commit (string-append "v" version))))
> +              (file-name (git-file-name name version))
> +              (sha256
> +               (base32
> +                "0pqj7kyyzr1zs4h9yzn5rdxnxspm3wqgsv00765dd42fszlmrmk8"))
> +              (patches (search-patches "llhttp-bootstrap-CVE-2020-8287.patch"))
> +              (modules '((guix build utils)))
> +              (snippet
> +               '(begin
> +                  ;; Fix imports for esbuild.
> +                  ;; https://github.com/evanw/esbuild/issues/477
> +                  (substitute* "src/llhttp/http.ts"
> +                    (("\\* as assert") "assert"))
> +                  (substitute* "Makefile"
> +                    (("npx ts-node bin/generate.ts")
> +                     "node bin/generate.js"))
> +                  #t))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:tests? #f                      ; no tests
> +       #:make-flags (list "CLANG=gcc"

This should probably be cc-for-target

> +                          (string-append "DESTDIR=" (assoc-ref %outputs "out"))
> +                          "PREFIX=")

And normally DESTDIR is empty and PREFIX is %out. Does it need to be
switched here?

> +       #:phases
> +       (modify-phases %standard-phases
> +         (replace 'configure
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let ((esbuild (string-append (assoc-ref inputs "esbuild")
> +                                           "/bin/esbuild")))
> +               (invoke esbuild
> +                       "--platform=node"
> +                       "--outfile=bin/generate.js"
> +                       "--bundle" "bin/generate.ts"))))
> +         (add-before 'install 'create-install-directories
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let ((out (assoc-ref outputs "out")))
> +               (for-each (lambda (dir)
> +                           (mkdir-p (string-append out dir)))
> +                         (list "/lib" "/include" "/src"))
> +               #t)))
> +         (add-after 'install 'install-src
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (src-dir (string-append out "/src")))
> +               (install-file "build/c/llhttp.c" src-dir)
> +               (install-file "src/native/api.c" src-dir)
> +               (install-file "src/native/http.c" src-dir)
> +               #t))))))
> +    (native-inputs
> +     `(("esbuild" ,esbuild)
> +       ("node" ,node-bootstrap)
> +       ("node-semver" ,node-semver-bootstrap)
> +       ("node-llparse-bootstrap" ,node-llparse-bootstrap)))
> +    (home-page "https://github.com/nodejs/llhttp")
> +    (properties '((hidden? . #t)))
> +    (synopsis "Parser for HTTP messages")
> +    (description "This is a rewrite of
> +@url{https://github.com/nodejs/http-parser, http-parser} using
> +@url{https://github.com/nodejs/llparse, llparse} to generate the C
> +source files.")
> +    (license license:expat)))
> +
>  (define-public libnode
>    (package/inherit node
>      (name "libnode")
> diff --git a/gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch b/gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch
> new file mode 100644
> index 0000000000..215c920e53
> --- /dev/null
> +++ b/gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch
> @@ -0,0 +1,100 @@
> +This patch comes from upstream.  It corresponds to a patch applied to
> +the generated C source code for llhttp included in Node.js 14.16.0
> +(see commit 641f786bb1a1f6eb1ff8750782ed939780f2b31a).  That commit
> +fixes CVE-2020-8287.  With this patch, the output of our
> +llhttp-bootstrap package matches the files included in Node.js 14.16.0
> +exactly.
> +
> +commit e9b36ea64709c35ca66094d5cf3787f444029601
> +Author: Fedor Indutny <fedor@indutny.com>
> +Date:   Sat Oct 10 19:56:01 2020 -0700
> +
> +    http: unset `F_CHUNKED` on new `Transfer-Encoding`
> +    
> +    Duplicate `Transfer-Encoding` header should be a treated as a single,
> +    but with original header values concatenated with a comma separator. In
> +    the light of this, even if the past `Transfer-Encoding` ended with
> +    `chunked`, we should be not let the `F_CHUNKED` to leak into the next
> +    header, because mere presence of another header indicates that `chunked`
> +    is not the last transfer-encoding token.
> +
> +diff --git a/src/llhttp/http.ts b/src/llhttp/http.ts
> +index f4f1a6e..0a0c365 100644
> +--- a/src/llhttp/http.ts
> ++++ b/src/llhttp/http.ts
> +@@ -460,11 +460,19 @@ export class HTTP {
> +       .match([ ' ', '\t' ], n('header_value_discard_ws'))
> +       .otherwise(checkContentLengthEmptiness);
> + 
> ++    // Multiple `Transfer-Encoding` headers should be treated as one, but with
> ++    // values separate by a comma.
> ++    //
> ++    // See: https://tools.ietf.org/html/rfc7230#section-3.2.2
> ++    const toTransferEncoding = this.unsetFlag(
> ++      FLAGS.CHUNKED,
> ++      'header_value_te_chunked');
> ++
> +     n('header_value_start')
> +       .otherwise(this.load('header_state', {
> +         [HEADER_STATE.UPGRADE]: this.setFlag(FLAGS.UPGRADE, fallback),
> +         [HEADER_STATE.TRANSFER_ENCODING]: this.setFlag(
> +-          FLAGS.TRANSFER_ENCODING, 'header_value_te_chunked'),
> ++          FLAGS.TRANSFER_ENCODING, toTransferEncoding),
> +         [HEADER_STATE.CONTENT_LENGTH]: n('header_value_content_length_once'),
> +         [HEADER_STATE.CONNECTION]: n('header_value_connection'),
> +       }, 'header_value'));
> +@@ -847,6 +855,11 @@ export class HTTP {
> +     return span.start(span.end(this.node(next)));
> +   }
> + 
> ++  private unsetFlag(flag: FLAGS, next: string | Node): Node {
> ++    const p = this.llparse;
> ++    return p.invoke(p.code.and('flags', ~flag), this.node(next));
> ++  }
> ++
> +   private setFlag(flag: FLAGS, next: string | Node): Node {
> +     const p = this.llparse;
> +     return p.invoke(p.code.or('flags', flag), this.node(next));
> +diff --git a/test/request/transfer-encoding.md b/test/request/transfer-encoding.md
> +index a7d1681..b0891d6 100644
> +--- a/test/request/transfer-encoding.md
> ++++ b/test/request/transfer-encoding.md
> +@@ -353,6 +353,38 @@ off=106 headers complete method=3 v=1/1 flags=200 content_length=0
> + off=106 error code=15 reason="Request has invalid `Transfer-Encoding`"
> + ```
> + 
> ++## POST with `chunked` and duplicate transfer-encoding
> ++
> ++<!-- meta={"type": "request", "noScan": true} -->
> ++```http
> ++POST /post_identity_body_world?q=search#hey HTTP/1.1
> ++Accept: */*
> ++Transfer-Encoding: chunked
> ++Transfer-Encoding: deflate
> ++
> ++World
> ++```
> ++
> ++```log
> ++off=0 message begin
> ++off=5 len=38 span[url]="/post_identity_body_world?q=search#hey"
> ++off=44 url complete
> ++off=54 len=6 span[header_field]="Accept"
> ++off=61 header_field complete
> ++off=62 len=3 span[header_value]="*/*"
> ++off=67 header_value complete
> ++off=67 len=17 span[header_field]="Transfer-Encoding"
> ++off=85 header_field complete
> ++off=86 len=7 span[header_value]="chunked"
> ++off=95 header_value complete
> ++off=95 len=17 span[header_field]="Transfer-Encoding"
> ++off=113 header_field complete
> ++off=114 len=7 span[header_value]="deflate"
> ++off=123 header_value complete
> ++off=125 headers complete method=3 v=1/1 flags=200 content_length=0
> ++off=125 error code=15 reason="Request has invalid `Transfer-Encoding`"
> ++```
> ++
> + ## POST with `chunked` before other transfer-coding (lenient)
> + 
> + TODO(indutny): should we allow it even in lenient mode? (Consider disabling
> -- 
> 2.31.0
> 
> 
> 
>
Jelle Licht April 2, 2021, 1:17 p.m. UTC | #2 
Efraim Flashner <efraim@flashner.co.il> writes:

> On Tue, Mar 30, 2021 at 01:27:42AM -0400, Timothy Sample wrote:
>> +    (build-system gnu-build-system)
>> +    (arguments
>> +     `(#:tests? #f                      ; no tests
>> +       #:make-flags (list "CLANG=gcc"
>
> This should probably be cc-for-target

Fixed in my local version, thanks.

>> +                          (string-append "DESTDIR=" (assoc-ref %outputs "out"))
>> +                          "PREFIX=")
>
> And normally DESTDIR is empty and PREFIX is %out. Does it need to be
> switched here?

It does.

 - Jelle
diff mbox series

Patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 52a021c2a3..5959a563d1 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1366,6 +1366,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/linux-pam-no-setfsuid.patch		\
   %D%/packages/patches/lirc-localstatedir.patch			\
   %D%/packages/patches/lirc-reproducible-build.patch		\
+  %D%/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch	\
   %D%/packages/patches/llvm-3.5-fix-clang-build-with-gcc5.patch	\
   %D%/packages/patches/llvm-9-fix-bitcast-miscompilation.patch	\
   %D%/packages/patches/llvm-9-fix-lpad-miscompilation.patch	\
diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
index 5336012e43..45e5f8feca 100644
--- a/gnu/packages/node.scm
+++ b/gnu/packages/node.scm
@@ -510,6 +510,76 @@  Node.js and web browsers.")
 parser definition into a C output.")
     (license license:expat)))
 
+(define-public llhttp-bootstrap
+  (package
+    (name "llhttp")
+    (version "2.1.3")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url "https://github.com/nodejs/llhttp.git")
+                    (commit (string-append "v" version))))
+              (file-name (git-file-name name version))
+              (sha256
+               (base32
+                "0pqj7kyyzr1zs4h9yzn5rdxnxspm3wqgsv00765dd42fszlmrmk8"))
+              (patches (search-patches "llhttp-bootstrap-CVE-2020-8287.patch"))
+              (modules '((guix build utils)))
+              (snippet
+               '(begin
+                  ;; Fix imports for esbuild.
+                  ;; https://github.com/evanw/esbuild/issues/477
+                  (substitute* "src/llhttp/http.ts"
+                    (("\\* as assert") "assert"))
+                  (substitute* "Makefile"
+                    (("npx ts-node bin/generate.ts")
+                     "node bin/generate.js"))
+                  #t))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:tests? #f                      ; no tests
+       #:make-flags (list "CLANG=gcc"
+                          (string-append "DESTDIR=" (assoc-ref %outputs "out"))
+                          "PREFIX=")
+       #:phases
+       (modify-phases %standard-phases
+         (replace 'configure
+           (lambda* (#:key inputs #:allow-other-keys)
+             (let ((esbuild (string-append (assoc-ref inputs "esbuild")
+                                           "/bin/esbuild")))
+               (invoke esbuild
+                       "--platform=node"
+                       "--outfile=bin/generate.js"
+                       "--bundle" "bin/generate.ts"))))
+         (add-before 'install 'create-install-directories
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let ((out (assoc-ref outputs "out")))
+               (for-each (lambda (dir)
+                           (mkdir-p (string-append out dir)))
+                         (list "/lib" "/include" "/src"))
+               #t)))
+         (add-after 'install 'install-src
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out"))
+                    (src-dir (string-append out "/src")))
+               (install-file "build/c/llhttp.c" src-dir)
+               (install-file "src/native/api.c" src-dir)
+               (install-file "src/native/http.c" src-dir)
+               #t))))))
+    (native-inputs
+     `(("esbuild" ,esbuild)
+       ("node" ,node-bootstrap)
+       ("node-semver" ,node-semver-bootstrap)
+       ("node-llparse-bootstrap" ,node-llparse-bootstrap)))
+    (home-page "https://github.com/nodejs/llhttp")
+    (properties '((hidden? . #t)))
+    (synopsis "Parser for HTTP messages")
+    (description "This is a rewrite of
+@url{https://github.com/nodejs/http-parser, http-parser} using
+@url{https://github.com/nodejs/llparse, llparse} to generate the C
+source files.")
+    (license license:expat)))
+
 (define-public libnode
   (package/inherit node
     (name "libnode")
diff --git a/gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch b/gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch
new file mode 100644
index 0000000000..215c920e53
--- /dev/null
+++ b/gnu/packages/patches/llhttp-bootstrap-CVE-2020-8287.patch
@@ -0,0 +1,100 @@ 
+This patch comes from upstream.  It corresponds to a patch applied to
+the generated C source code for llhttp included in Node.js 14.16.0
+(see commit 641f786bb1a1f6eb1ff8750782ed939780f2b31a).  That commit
+fixes CVE-2020-8287.  With this patch, the output of our
+llhttp-bootstrap package matches the files included in Node.js 14.16.0
+exactly.
+
+commit e9b36ea64709c35ca66094d5cf3787f444029601
+Author: Fedor Indutny <fedor@indutny.com>
+Date:   Sat Oct 10 19:56:01 2020 -0700
+
+    http: unset `F_CHUNKED` on new `Transfer-Encoding`
+    
+    Duplicate `Transfer-Encoding` header should be a treated as a single,
+    but with original header values concatenated with a comma separator. In
+    the light of this, even if the past `Transfer-Encoding` ended with
+    `chunked`, we should be not let the `F_CHUNKED` to leak into the next
+    header, because mere presence of another header indicates that `chunked`
+    is not the last transfer-encoding token.
+
+diff --git a/src/llhttp/http.ts b/src/llhttp/http.ts
+index f4f1a6e..0a0c365 100644
+--- a/src/llhttp/http.ts
++++ b/src/llhttp/http.ts
+@@ -460,11 +460,19 @@ export class HTTP {
+       .match([ ' ', '\t' ], n('header_value_discard_ws'))
+       .otherwise(checkContentLengthEmptiness);
+ 
++    // Multiple `Transfer-Encoding` headers should be treated as one, but with
++    // values separate by a comma.
++    //
++    // See: https://tools.ietf.org/html/rfc7230#section-3.2.2
++    const toTransferEncoding = this.unsetFlag(
++      FLAGS.CHUNKED,
++      'header_value_te_chunked');
++
+     n('header_value_start')
+       .otherwise(this.load('header_state', {
+         [HEADER_STATE.UPGRADE]: this.setFlag(FLAGS.UPGRADE, fallback),
+         [HEADER_STATE.TRANSFER_ENCODING]: this.setFlag(
+-          FLAGS.TRANSFER_ENCODING, 'header_value_te_chunked'),
++          FLAGS.TRANSFER_ENCODING, toTransferEncoding),
+         [HEADER_STATE.CONTENT_LENGTH]: n('header_value_content_length_once'),
+         [HEADER_STATE.CONNECTION]: n('header_value_connection'),
+       }, 'header_value'));
+@@ -847,6 +855,11 @@ export class HTTP {
+     return span.start(span.end(this.node(next)));
+   }
+ 
++  private unsetFlag(flag: FLAGS, next: string | Node): Node {
++    const p = this.llparse;
++    return p.invoke(p.code.and('flags', ~flag), this.node(next));
++  }
++
+   private setFlag(flag: FLAGS, next: string | Node): Node {
+     const p = this.llparse;
+     return p.invoke(p.code.or('flags', flag), this.node(next));
+diff --git a/test/request/transfer-encoding.md b/test/request/transfer-encoding.md
+index a7d1681..b0891d6 100644
+--- a/test/request/transfer-encoding.md
++++ b/test/request/transfer-encoding.md
+@@ -353,6 +353,38 @@ off=106 headers complete method=3 v=1/1 flags=200 content_length=0
+ off=106 error code=15 reason="Request has invalid `Transfer-Encoding`"
+ ```
+ 
++## POST with `chunked` and duplicate transfer-encoding
++
++<!-- meta={"type": "request", "noScan": true} -->
++```http
++POST /post_identity_body_world?q=search#hey HTTP/1.1
++Accept: */*
++Transfer-Encoding: chunked
++Transfer-Encoding: deflate
++
++World
++```
++
++```log
++off=0 message begin
++off=5 len=38 span[url]="/post_identity_body_world?q=search#hey"
++off=44 url complete
++off=54 len=6 span[header_field]="Accept"
++off=61 header_field complete
++off=62 len=3 span[header_value]="*/*"
++off=67 header_value complete
++off=67 len=17 span[header_field]="Transfer-Encoding"
++off=85 header_field complete
++off=86 len=7 span[header_value]="chunked"
++off=95 header_value complete
++off=95 len=17 span[header_field]="Transfer-Encoding"
++off=113 header_field complete
++off=114 len=7 span[header_value]="deflate"
++off=123 header_value complete
++off=125 headers complete method=3 v=1/1 flags=200 content_length=0
++off=125 error code=15 reason="Request has invalid `Transfer-Encoding`"
++```
++
+ ## POST with `chunked` before other transfer-coding (lenient)
+ 
+ TODO(indutny): should we allow it even in lenient mode? (Consider disabling