Repository: miekg/dns
Pull request #1150 (2020-08-13 15:31:03)

make sure removing trailing zero octets in APL AFDPART
The current implementation of `APL.pack` violates the following part of RFC3123 (Sections 4.1 and 4.2):
> the sender MUST NOT include trailing zero octets in the AFDPART regardless of the value of PREFIX

This PR implements a trivial fix to this violation.

codecov-commenter: # [Codecov](https://codecov.io/gh/miekg/dns/pull/1150?src=pr&el=h1) Report
> Merging [#1150](https://codecov.io/gh/miekg/dns/pull/1150?src=pr&el=desc) into [master](https://codecov.io/gh/miekg/dns/commit/34cecfe1b489442934480cec58277b4b39b1a1cd&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/miekg/dns/pull/1150/graphs/tree.svg?width=650&height=150&src=pr&token=uT9fYPSeKT)](https://codecov.io/gh/miekg/dns/pull/1150?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1150      +/-   ##
==========================================
+ Coverage   55.56%   55.57%   +0.01%     
==========================================
  Files          41       41              
  Lines       10124    10127       +3     
==========================================
+ Hits         5625     5628       +3     
  Misses       3476     3476              
  Partials     1023     1023              
```


| [Impacted Files](https://codecov.io/gh/miekg/dns/pull/1150?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [msg\_helpers.go](https://codecov.io/gh/miekg/dns/pull/1150/diff?src=pr&el=tree#diff-bXNnX2hlbHBlcnMuZ28=) | `68.20% <100.00%> (+0.19%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/miekg/dns/pull/1150?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/miekg/dns/pull/1150?src=pr&el=footer). Last update [34cecfe...b661c9d](https://codecov.io/gh/miekg/dns/pull/1150?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/msg_helpers.go b/msg_helpers.go
index cbcab57b..6fb64475 100644
--- a/msg_helpers.go
+++ b/msg_helpers.go
@@ -683,6 +683,13 @@ func packDataAplPrefix(p *APLPrefix, msg []byte, off int) (int, error) {
 	if p.Negation {
 		n = 0x80
 	}
+
+	// trim trailing zero bytes as specified in RFC3123 Sections 4.1 and 4.2.
+	i := len(addr) - 1
+	for ; i >= 0 && addr[i] == 0; i-- {
+	}
+	addr = addr[:i+1]
+
 	adflen := uint8(len(addr)) & 0x7f
 	off, err = packUint8(n|adflen, msg, off)
 	if err != nil {
diff --git a/scan.go b/scan.go
index e18566fc..aa2840ef 100644
--- a/scan.go
+++ b/scan.go
@@ -1210,11 +1210,29 @@ func stringToCm(token string) (e, m uint8, ok bool) {
 		if cmeters, err = strconv.Atoi(s[1]); err != nil {
 			return
 		}
+		// There's no point in having more than 2 digits in this part, and would rather make the implementation complicated ('123' should be treated as '12').
+		// So we simply reject it.
+		// We also make sure the first character is a digit to reject '+-' signs.
+		if len(s[1]) > 2 || s[1][0] < '0' || s[1][0] > '9' {
+			return
+		}
+		if len(s[1]) == 1 {
+			// 'nn.1' must be treated as 'nn-meters and 10cm, not 1cm.
+			cmeters *= 10
+		}
+		if len(s[0]) == 0 {
+			// This will allow omitting the 'meter' part, like .01 (meaning 0.01m = 1cm).
+			break
+		}
 		fallthrough
 	case 1:
 		if meters, err = strconv.Atoi(s[0]); err != nil {
 			return
 		}
+		// RFC1876 states the max value is 90000000.00.  The latter two conditions enforce it.
+		if s[0][0] < '0' || s[0][0] > '9' || meters > 90000000 || (meters == 90000000 && cmeters != 0) {
+			return
+		}
 	case 0:
 		// huh?
 		return 0, 0, false
@@ -1227,13 +1245,10 @@ func stringToCm(token string) (e, m uint8, ok bool) {
 		e = 0
 		val = cmeters
 	}
-	for val > 10 {
+	for val >= 10 {
 		e++
 		val /= 10
 	}
-	if e > 9 {
-		ok = false
-	}
 	m = uint8(val)
 	return
 }
@@ -1275,6 +1290,9 @@ func appendOrigin(name, origin string) string {
 
 // LOC record helper function
 func locCheckNorth(token string, latitude uint32) (uint32, bool) {
+	if latitude > 90 * 1000 * 60 * 60 {
+		return latitude, false
+	}
 	switch token {
 	case "n", "N":
 		return LOC_EQUATOR + latitude, true
@@ -1286,6 +1304,9 @@ func locCheckNorth(token string, latitude uint32) (uint32, bool) {
 
 // LOC record helper function
 func locCheckEast(token string, longitude uint32) (uint32, bool) {
+	if longitude > 180 * 1000 * 60 * 60 {
+		return longitude, false
+	}
 	switch token {
 	case "e", "E":
 		return LOC_EQUATOR + longitude, true
diff --git a/scan_rr.go b/scan_rr.go
index 11b08ad1..69f10052 100644
--- a/scan_rr.go
+++ b/scan_rr.go
@@ -590,7 +590,7 @@ func (rr *LOC) parse(c *zlexer, o string) *ParseError {
 	// North
 	l, _ := c.Next()
 	i, e := strconv.ParseUint(l.token, 10, 32)
-	if e != nil || l.err {
+	if e != nil || l.err || i > 90 {
 		return &ParseError{"", "bad LOC Latitude", l}
 	}
 	rr.Latitude = 1000 * 60 * 60 * uint32(i)
@@ -601,7 +601,7 @@ func (rr *LOC) parse(c *zlexer, o string) *ParseError {
 	if rr.Latitude, ok = locCheckNorth(l.token, rr.Latitude); ok {
 		goto East
 	}
-	if i, err := strconv.ParseUint(l.token, 10, 32); err != nil || l.err {
+	if i, err := strconv.ParseUint(l.token, 10, 32); err != nil || l.err || i > 59 {
 		return &ParseError{"", "bad LOC Latitude minutes", l}
 	} else {
 		rr.Latitude += 1000 * 60 * uint32(i)
@@ -609,7 +609,7 @@ func (rr *LOC) parse(c *zlexer, o string) *ParseError {
 
 	c.Next() // zBlank
 	l, _ = c.Next()
-	if i, err := strconv.ParseFloat(l.token, 32); err != nil || l.err {
+	if i, err := strconv.ParseFloat(l.token, 32); err != nil || l.err || i < 0 || i >= 60 {
 		return &ParseError{"", "bad LOC Latitude seconds", l}
 	} else {
 		rr.Latitude += uint32(1000 * i)
@@ -627,7 +627,7 @@ East:
 	// East
 	c.Next() // zBlank
 	l, _ = c.Next()
-	if i, err := strconv.ParseUint(l.token, 10, 32); err != nil || l.err {
+	if i, err := strconv.ParseUint(l.token, 10, 32); err != nil || l.err || i > 180 {
 		return &ParseError{"", "bad LOC Longitude", l}
 	} else {
 		rr.Longitude = 1000 * 60 * 60 * uint32(i)
@@ -638,14 +638,14 @@ East:
 	if rr.Longitude, ok = locCheckEast(l.token, rr.Longitude); ok {
 		goto Altitude
 	}
-	if i, err := strconv.ParseUint(l.token, 10, 32); err != nil || l.err {
+	if i, err := strconv.ParseUint(l.token, 10, 32); err != nil || l.err || i > 59 {
 		return &ParseError{"", "bad LOC Longitude minutes", l}
 	} else {
 		rr.Longitude += 1000 * 60 * uint32(i)
 	}
 	c.Next() // zBlank
 	l, _ = c.Next()
-	if i, err := strconv.ParseFloat(l.token, 32); err != nil || l.err {
+	if i, err := strconv.ParseFloat(l.token, 32); err != nil || l.err || i < 0 || i >= 60 {
 		return &ParseError{"", "bad LOC Longitude seconds", l}
 	} else {
 		rr.Longitude += uint32(1000 * i)
@@ -668,7 +668,7 @@ Altitude:
 	if l.token[len(l.token)-1] == 'M' || l.token[len(l.token)-1] == 'm' {
 		l.token = l.token[0 : len(l.token)-1]
 	}
-	if i, err := strconv.ParseFloat(l.token, 32); err != nil {
+	if i, err := strconv.ParseFloat(l.token, 64); err != nil {
 		return &ParseError{"", "bad LOC Altitude", l}
 	} else {
 		rr.Altitude = uint32(i*100.0 + 10000000.0 + 0.5)
@@ -893,8 +893,7 @@ func (rr *RRSIG) parse(c *zlexer, o string) *ParseError {
 	l, _ = c.Next()
 	if i, err := StringToTime(l.token); err != nil {
 		// Try to see if all numeric and use it as epoch
-		if i, err := strconv.ParseInt(l.token, 10, 64); err == nil {
-			// TODO(miek): error out on > MAX_UINT32, same below
+		if i, err := strconv.ParseUint(l.token, 10, 32); err == nil {
 			rr.Expiration = uint32(i)
 		} else {
 			return &ParseError{"", "bad RRSIG Expiration", l}
@@ -906,7 +905,7 @@ func (rr *RRSIG) parse(c *zlexer, o string) *ParseError {
 	c.Next() // zBlank
 	l, _ = c.Next()
 	if i, err := StringToTime(l.token); err != nil {
-		if i, err := strconv.ParseInt(l.token, 10, 64); err == nil {
+		if i, err := strconv.ParseUint(l.token, 10, 32); err == nil {
 			rr.Inception = uint32(i)
 		} else {
 			return &ParseError{"", "bad RRSIG Inception", l}


Test patch:
diff --git a/msg_helpers_test.go b/msg_helpers_test.go
index 70e3b238..c1fa195a 100644
--- a/msg_helpers_test.go
+++ b/msg_helpers_test.go
@@ -180,6 +180,20 @@ func TestPackDataAplPrefix(t *testing.T) {
 			net.CIDRMask(48, 128),
 			[]byte{0x00, 0x02, 0x30, 0x06, 0x20, 0x01, 0x0d, 0xb8, 0xca, 0xfe},
 		},
+		{
+			"with trailing zero bytes 2:2001:db8:cafe::0/64",
+			false,
+			net.ParseIP("2001:db8:cafe::"),
+			net.CIDRMask(64, 128),
+			[]byte{0x00, 0x02, 0x40, 0x06, 0x20, 0x01, 0x0d, 0xb8, 0xca, 0xfe},
+		},
+		{
+			"no non-zero bytes 2::/16",
+			false,
+			net.ParseIP("::"),
+			net.CIDRMask(16, 128),
+			[]byte{0x00, 0x02, 0x10, 0x00},
+		},
 		{
 			"!2:2001:db8::/32",
 			true,
@@ -209,6 +223,11 @@ func TestPackDataAplPrefix(t *testing.T) {
 			if !bytes.Equal(tt.expect, out[:off]) {
 				t.Fatalf("expected output %02x, got %02x", tt.expect, out[:off])
 			}
+			// Make sure the packed bytes would be accepted by its own unpack
+			_, _, err = unpackDataAplPrefix(out, 0)
+			if err != nil {
+				t.Fatalf("expected no error, got %q", err)
+			}
 		})
 	}
 }
diff --git a/parse_test.go b/parse_test.go
index a6b82757..74c6279d 100644
--- a/parse_test.go
+++ b/parse_test.go
@@ -376,6 +376,9 @@ func TestParseLOC(t *testing.T) {
 		"SW1A2AA.find.me.uk.	LOC	51 30 12.748 N 00 07 39.611 W 0.00m 0.00m 0.00m 0.00m": "SW1A2AA.find.me.uk.\t3600\tIN\tLOC\t51 30 12.748 N 00 07 39.611 W 0m 0.00m 0.00m 0.00m",
 		"SW1A2AA.find.me.uk.	LOC	51 0 0.0 N 00 07 39.611 W 0.00m 0.00m 0.00m 0.00m": "SW1A2AA.find.me.uk.\t3600\tIN\tLOC\t51 00 0.000 N 00 07 39.611 W 0m 0.00m 0.00m 0.00m",
 		"SW1A2AA.find.me.uk.	LOC	51 30 12.748 N 00 07 39.611 W 0.00m": "SW1A2AA.find.me.uk.\t3600\tIN\tLOC\t51 30 12.748 N 00 07 39.611 W 0m 1m 10000m 10m",
+		// Exercise boundary cases
+		"SW1A2AA.find.me.uk.	LOC	90 0 0.0 N 180 0 0.0 W 42849672.95 90000000.00m 90000000.00m 90000000.00m": "SW1A2AA.find.me.uk.\t3600\tIN\tLOC\t90 00 0.000 N 180 00 0.000 W 42849672.95m 90000000m 90000000m 90000000m",
+		"SW1A2AA.find.me.uk.	LOC	89 59 59.999 N 179 59 59.999 W -100000 90000000.00m 90000000.00m 90000000m": "SW1A2AA.find.me.uk.\t3600\tIN\tLOC\t89 59 59.999 N 179 59 59.999 W -100000m 90000000m 90000000m 90000000m",
 	}
 	for i, o := range lt {
 		rr, err := NewRR(i)
@@ -387,6 +390,90 @@ func TestParseLOC(t *testing.T) {
 			t.Errorf("`%s' should be equal to\n`%s', but is     `%s'", i, o, rr.String())
 		}
 	}
+
+	// Invalid cases (out of range values)
+	lt = map[string]string{ // Pair of (invalid) RDATA and the bad field name
+		// One of the subfields is out of range.
+		"91 0 0.0 N 00 07 39.611 W 0m":   "Latitude",
+		"89 60 0.0 N 00 07 39.611 W 0m":  "Latitude",
+		"89 00 60.0 N 00 07 39.611 W 0m": "Latitude",
+		"1 00 -1 N 00 07 39.611 W 0m":    "Latitude",
+		"0 0 0.0 N 181 00 0.0 W 0m":      "Longitude",
+		"0 0 0.0 N 179 60 0.0 W 0m":      "Longitude",
+		"0 0 0.0 N 179 00 60.0 W 0m":     "Longitude",
+		"0 0 0.0 N 1 00 -1 W 0m":         "Longitude",
+
+		// Each subfield is valid, but resulting latitude would be out of range.
+		"90 01 00.0 N 00 07 39.611 W 0m": "Latitude",
+		"0 0 0.0 N 180 01 0.0 W 0m":      "Longitude",
+	}
+	for rdata, field := range lt {
+		_, err := NewRR(fmt.Sprintf("example.com. LOC %s", rdata))
+		if err == nil || !strings.Contains(err.Error(), field) {
+			t.Errorf("expected error to contain %q, but got %v", field, err)
+		}
+	}
+}
+
+// this tests a subroutine for the LOC RR parser.  It's complicated enough to test separately.
+func TestStringToCm(t *testing.T) {
+	tests := []struct {
+		// Test description: the input token and the expected return values from stringToCm.
+		token string
+		e     uint8
+		m     uint8
+		ok    bool
+	}{
+		{"100", 4, 1, true},
+		{"0100", 4, 1, true}, // leading 0 (allowed)
+		{"100.99", 4, 1, true},
+		{"90000000", 9, 9, true},
+		{"90000000.00", 9, 9, true},
+		{"0", 0, 0, true},
+		{"0.00", 0, 0, true},
+		{"0.01", 0, 1, true},
+		{".01", 0, 1, true}, // empty 'meter' part (allowed)
+		{"0.1", 1, 1, true},
+
+		// out of range (too large)
+		{"90000001", 0, 0, false},
+		{"90000000.01", 0, 0, false},
+
+		// more than 2 digits in 'cmeter' part
+		{"0.000", 0, 0, false},
+		{"0.001", 0, 0, false},
+		{"0.999", 0, 0, false},
+		// with plus or minus sign (disallowed)
+		{"-100", 0, 0, false},
+		{"+100", 0, 0, false},
+		{"0.-10", 0, 0, false},
+		{"0.+10", 0, 0, false},
+		{"0a.00", 0, 0, false}, // invalid string for 'meter' part
+		{".1x", 0, 0, false},   // invalid string for 'cmeter' part
+		{".", 0, 0, false},     // empty 'cmeter' part (disallowed)
+		{"1.", 0, 0, false},    // ditto
+		{"m", 0, 0, false},     // only the "m" suffix
+	}
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.token, func(t *testing.T) {
+			// In all cases the expected result is the same with or without the 'm' suffix.
+			// So we test both cases using the same test code.
+			for _, sfx := range []string{"", "m"} {
+				token := tc.token + sfx
+				e, m, ok := stringToCm(token)
+				if ok != tc.ok {
+					t.Fatal("unexpected validation result")
+				}
+				if m != tc.m {
+					t.Fatalf("Expected %d, got %d", tc.m, m)
+				}
+				if e != tc.e {
+					t.Fatalf("Expected %d, got %d", tc.e, e)
+				}
+			}
+		})
+	}
 }
 
 func TestParseDS(t *testing.T) {