Pull request: 2305 limit message size

Merge in DNS/adguard-home from 2305-limit-message-size to master

Closes #2305.

Squashed commit of the following:

commit 6edd1e0
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Mon Nov 23 14:03:36 2020 +0300

    aghio: fix final inaccuracies

commit 4dd382a
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Mon Nov 23 13:59:10 2020 +0300

    all: improve code quality

commit 060f923
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Mon Nov 23 13:10:57 2020 +0300

    aghio: add validation to constructor

commit f57a2f5
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Fri Nov 20 19:19:26 2020 +0300

    all: fix minor inaccuracies

commit 93462c7
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Fri Nov 20 19:13:23 2020 +0300

    home: make test name follow convention

commit 4922986
Merge: 1f5472a 046ec13
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Fri Nov 20 19:09:01 2020 +0300

    Merge branch 'master' into 2305-limit-message-size

commit 1f5472a
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Fri Nov 20 19:08:21 2020 +0300

    aghio: improve readability

commit 60dc706
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Fri Nov 20 18:44:08 2020 +0300

    home: cover middleware with test

commit bedf436
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Fri Nov 20 17:10:23 2020 +0300

    aghio: improved error informativeness

commit 682c5da
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Fri Nov 20 13:37:51 2020 +0300

    all: limit readers for ReadAll dealing with miscellanious data.

commit 78c6dd8
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Nov 19 20:07:43 2020 +0300

    all: handle ReadAll calls dealing with request's bodies.

commit bfe1a6f
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Nov 19 17:25:34 2020 +0300

    home: add middlewares

commit bbd1d49
Merge: 7b77c2c 62a8fe0
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Nov 19 16:44:04 2020 +0300

    Merge branch 'master' into 2305-limit-message-size

commit 7b77c2c
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue Nov 17 15:33:33 2020 +0300

    aghio: create package

CHANGELOG.md

@@ -9,6 +9,19 @@ and this project adheres to
 
 ## [Unreleased]
 
+### Added
+
+- HTTP API request body size limit [#2305].
+
+[#2305]: https://github.com/AdguardTeam/AdGuardHome/issues/2305
+
+### Changed
+
+- Various internal improvements ([#2271], [#2297]).
+
+[#2271]: https://github.com/AdguardTeam/AdGuardHome/issues/2271
+[#2297]: https://github.com/AdguardTeam/AdGuardHome/issues/2297
+
 
 
 ## [v0.104.3] - 2020-11-19

HACKING.md

@@ -1,12 +1,16 @@
- #  AdGuardHome Developer Guidelines
+ #  *AdGuardHome* Developer Guidelines
 
 As of **2020-11-20**, this document is still a work-in-progress.  Some of the
 rules aren't enforced, and others might change.  Still, this is a good place to
 find out about how we **want** our code to look like.
 
 The rules are mostly sorted in the alphabetical order.
 
-##  Git
+##  *Git*
+
+ *  Call your branches either `NNNN-fix-foo` (where `NNNN` is the ID of the
+    *GitHub* issue you worked on in this branch) or just `fix-foo` if there was
+    no *GitHub* issue.
 
  *  Follow the commit message header format:
 
@@ -22,16 +26,20 @@ The rules are mostly sorted in the alphabetical order.
  *  Only use lowercase letters in your commit message headers.  The rest of the
     message should follow the plain text conventions below.
 
-    The only exception are direct mentions of identifiers from the source code.
+    The only exceptions are direct mentions of identifiers from the source code
+    and filenames like `HACKING.md`.
 
-##  Go
+##  *Go*
 
  *  <https://github.com/golang/go/wiki/CodeReviewComments>.
 
  *  <https://github.com/golang/go/wiki/TestComments>.
 
  *  <https://go-proverbs.github.io/>
 
+ *  Add an empty line before `break`, `continue`, and `return`, unless it's the
+    only statement in that block.
+
  *  Avoid `init` and use explicit initialization functions instead.
 
  *  Avoid `new`, especially with structs.
@@ -53,6 +61,18 @@ The rules are mostly sorted in the alphabetical order.
  *  Eschew external dependencies, including transitive, unless
     absolutely necessary.
 
+ *  Name benchmarks and tests using the same convention as examples.  For
+    example:
+
+    ```go
+    func TestFunction(t *testing.T) { /* … */ }
+    func TestFunction_suffix(t *testing.T) { /* … */ }
+    func TestType_Method(t *testing.T) { /* … */ }
+    func TestType_Method_suffix(t *testing.T) { /* … */ }
+    ```
+
+ *  Name the deferred errors (e.g. when closing something) `cerr`.
+
  *  No `goto`.
 
  *  No shadowing, since it can often lead to subtle bugs, especially with
@@ -103,9 +123,9 @@ The rules are mostly sorted in the alphabetical order.
 [constant errors]: https://dave.cheney.net/2016/04/07/constant-errors
 [Linus said]:      https://www.kernel.org/doc/html/v4.17/process/coding-style.html#indentation
 
-##  Markdown
+##  *Markdown*
 
- *  **TODO(a.garipov):** Define our Markdown conventions.
+ *  **TODO(a.garipov):** Define our *Markdown* conventions.
 
 ##  Text, Including Comments
 
@@ -128,7 +148,7 @@ The rules are mostly sorted in the alphabetical order.
 
  *  Use double spacing between sentences to make sentence borders more clear.
 
- *  Use the serial comma (a.k.a. Oxford comma) to improve comprehension,
+ *  Use the serial comma (a.k.a. *Oxford* comma) to improve comprehension,
     decrease ambiguity, and use a common standard.
 
  *  Write todos like this:
@@ -143,16 +163,16 @@ The rules are mostly sorted in the alphabetical order.
     // TODO(usr1, usr2): Fix the frobulation issue.
     ```
 
-##  YAML
+##  *YAML*
 
  *  **TODO(a.garipov):** Define naming conventions for schema names in our
-    OpenAPI YAML file.  And just generally OpenAPI conventions.
+    *OpenAPI* *YAML* file.  And just generally OpenAPI conventions.
 
- *  **TODO(a.garipov):** Find a YAML formatter or write our own.
+ *  **TODO(a.garipov):** Find a *YAML* formatter or write our own.
 
- *  All strings, including keys, must be quoted.  Reason: the [NO-rway Law].
+ *  All strings, including keys, must be quoted.  Reason: the [*NO-rway Law*].
 
- *  Indent with two (**2**) spaces.  YAML documents can get pretty
+ *  Indent with two (**2**) spaces.  *YAML* documents can get pretty
     deeply-nested.
 
  *  No extra indentation in multiline arrays:
@@ -170,4 +190,4 @@ The rules are mostly sorted in the alphabetical order.
 
  *  Use `>` for multiline strings, unless you need to keep the line breaks.
 
-[NO-rway Law]: https://news.ycombinator.com/item?id=17359376
+[*NO-rway Law*]: https://news.ycombinator.com/item?id=17359376

internal/aghio/limitedreadcloser.go

@@ -0,0 +1,59 @@
+// Package aghio contains extensions for io package's types and methods
+package aghio
+
+import (
+	"fmt"
+	"io"
+)
+
+// LimitReachedError records the limit and the operation that caused it.
+type LimitReachedError struct {
+	Limit int64
+}
+
+// Error implements error interface for LimitReachedError.
+// TODO(a.garipov): Think about error string format.
+func (lre *LimitReachedError) Error() string {
+	return fmt.Sprintf("attempted to read more than %d bytes", lre.Limit)
+}
+
+// limitedReadCloser is a wrapper for io.ReadCloser with limited reader and
+// dealing with agherr package.
+type limitedReadCloser struct {
+	limit int64
+	n     int64
+	rc    io.ReadCloser
+}
+
+// Read implements Reader interface.
+func (lrc *limitedReadCloser) Read(p []byte) (n int, err error) {
+	if lrc.n == 0 {
+		return 0, &LimitReachedError{
+			Limit: lrc.limit,
+		}
+	}
+	if int64(len(p)) > lrc.n {
+		p = p[0:lrc.n]
+	}
+	n, err = lrc.rc.Read(p)
+	lrc.n -= int64(n)
+	return n, err
+}
+
+// Close implements Closer interface.
+func (lrc *limitedReadCloser) Close() error {
+	return lrc.rc.Close()
+}
+
+// LimitReadCloser wraps ReadCloser to make it's Reader stop with
+// ErrLimitReached after n bytes read.
+func LimitReadCloser(rc io.ReadCloser, n int64) (limited io.ReadCloser, err error) {
+	if n < 0 {
+		return nil, fmt.Errorf("aghio: invalid n in LimitReadCloser: %d", n)
+	}
+	return &limitedReadCloser{
+		limit: n,
+		n:     n,
+		rc:    rc,
+	}, nil
+}

internal/aghio/limitedreadcloser_test.go

@@ -0,0 +1,108 @@
+package aghio
+
+import (
+	"fmt"
+	"io"
+	"io/ioutil"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestLimitReadCloser(t *testing.T) {
+	testCases := []struct {
+		name string
+		n    int64
+		want error
+	}{{
+		name: "positive",
+		n:    1,
+		want: nil,
+	}, {
+		name: "zero",
+		n:    0,
+		want: nil,
+	}, {
+		name: "negative",
+		n:    -1,
+		want: fmt.Errorf("aghio: invalid n in LimitReadCloser: -1"),
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := LimitReadCloser(nil, tc.n)
+			assert.Equal(t, tc.want, err)
+		})
+	}
+}
+
+func TestLimitedReadCloser_Read(t *testing.T) {
+	testCases := []struct {
+		name  string
+		limit int64
+		rStr  string
+		want  int
+		err   error
+	}{{
+		name:  "perfectly_match",
+		limit: 3,
+		rStr:  "abc",
+		want:  3,
+		err:   nil,
+	}, {
+		name:  "eof",
+		limit: 3,
+		rStr:  "",
+		want:  0,
+		err:   io.EOF,
+	}, {
+		name:  "limit_reached",
+		limit: 0,
+		rStr:  "abc",
+		want:  0,
+		err: &LimitReachedError{
+			Limit: 0,
+		},
+	}, {
+		name:  "truncated",
+		limit: 2,
+		rStr:  "abc",
+		want:  2,
+		err:   nil,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			readCloser := ioutil.NopCloser(strings.NewReader(tc.rStr))
+			buf := make([]byte, tc.limit+1)
+
+			lreader, err := LimitReadCloser(readCloser, tc.limit)
+			assert.Nil(t, err)
+
+			n, err := lreader.Read(buf)
+			assert.Equal(t, n, tc.want)
+			assert.Equal(t, tc.err, err)
+		})
+	}
+}
+
+func TestLimitedReadCloser_LimitReachedError(t *testing.T) {
+	testCases := []struct {
+		name string
+		want string
+		err  error
+	}{{
+		name: "simplest",
+		want: "attempted to read more than 0 bytes",
+		err: &LimitReachedError{
+			Limit: 0,
+		},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, tc.err.Error())
+		})
+	}
+}

internal/dhcpd/dhcphttp.go

@@ -299,6 +299,7 @@ func (s *Server) handleDHCPInterfaces(w http.ResponseWriter, r *http.Request) {
 // . Check if a static IP is configured for the network interface
 // Respond with results
 func (s *Server) handleDHCPFindActiveServer(w http.ResponseWriter, r *http.Request) {
+	// This use of ReadAll is safe, because request's body is now limited.
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
 		msg := fmt.Sprintf("failed to read request body: %s", err)

internal/home/auth_glinet.go

@@ -10,6 +10,7 @@ import (
 	"time"
 	"unsafe"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghio"
 	"github.com/AdguardTeam/golibs/log"
 )
 
@@ -18,8 +19,10 @@ var GLMode bool
 
 var glFilePrefix = "/tmp/gl_token_"
 
-const glTokenTimeoutSeconds = 3600
-const glCookieName = "Admin-Token"
+const (
+	glTokenTimeoutSeconds = 3600
+	glCookieName          = "Admin-Token"
+)
 
 func glProcessRedirect(w http.ResponseWriter, r *http.Request) bool {
 	if !GLMode {
@@ -71,14 +74,28 @@ func archIsLittleEndian() bool {
 	return (b == 0x04)
 }
 
+// MaxFileSize is a maximum file length in bytes.
+const MaxFileSize = 1024 * 1024
+
 func glGetTokenDate(file string) uint32 {
 	f, err := os.Open(file)
 	if err != nil {
 		log.Error("os.Open: %s", err)
 		return 0
 	}
+	defer f.Close()
+
+	fileReadCloser, err := aghio.LimitReadCloser(f, MaxFileSize)
+	if err != nil {
+		log.Error("LimitReadCloser: %s", err)
+		return 0
+	}
+	defer fileReadCloser.Close()
+
 	var dateToken uint32
-	bs, err := ioutil.ReadAll(f)
+
+	// This use of ReadAll is now safe, because we limited reader.
+	bs, err := ioutil.ReadAll(fileReadCloser)
 	if err != nil {
 		log.Error("ioutil.ReadAll: %s", err)
 		return 0