Repository: miekg/dns
Pull request #1148 (2020-08-12 22:03:26)

fix and enhance stringToCm to parse LOC RR optional fields
`stringToCm`, a subroutine for `LOC.parse`, has a couple of issues that violate RFC1876:
- there's an off-by-one error in calculating the base and exponent values: a value of 100 should be encoded as 0x12 (1e2) but this implementation encodes it as 0xa1 (10e1).  (btw #1084 fixed the same problem but only for the default value)
- the current implementation treats "0.1m" as "1cm" (0x01 in the encoded form) as it simply extracts the number and passes it to `Atoi`.  But the actual value is 10cm and should be encoded as 0x11.
- the current implementation accepts a negative value

This PR fixes these issues.

It also introduces some other enhancements that are not necessarily for standard conformance:
- It enforces the max value (90000000.00m) described in the RFC more strictly.
- It allows omitting the 'meter' part if it follows by a dot and 'cmeter' part (e.g. ".01" to me 1cm)
- in addition to rejecting negative values, it also rejects somewhat awkward representation like "10.+10" (and also "+10.10", which may not necessarily be considered invalid, but probably not expected by the RFC)

I wouldn't necessarily insist on these enhancements (they are derived from the BIND's implementation of LOC).  If you don't like them please feel free to ignore them.

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

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

```diff
@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   55.56%   55.71%   +0.15%     
==========================================
  Files          41       41              
  Lines       10124    10130       +6     
==========================================
+ Hits         5625     5644      +19     
+ Misses       3476     3469       -7     
+ Partials     1023     1017       -6     
```


| [Impacted Files](https://codecov.io/gh/miekg/dns/pull/1148?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scan.go](https://codecov.io/gh/miekg/dns/pull/1148/diff?src=pr&el=tree#diff-c2Nhbi5nbw==) | `84.48% <100.00%> (+2.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/miekg/dns/pull/1148?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/1148?src=pr&el=footer). Last update [34cecfe...8e1be54](https://codecov.io/gh/miekg/dns/pull/1148?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

miekg: I've seen too many PRs in this area lately. Can any of those other authors
comment on this one?

On Thu, 13 Aug 2020, 00:10 Codecov Comments Bot, <notifications@github.com>
wrote:

> Codecov <https://codecov.io/gh/miekg/dns/pull/1148?src=pr&el=h1> Report
>
> Merging #1148 <https://codecov.io/gh/miekg/dns/pull/1148?src=pr&el=desc>
> into master
> <https://codecov.io/gh/miekg/dns/commit/34cecfe1b489442934480cec58277b4b39b1a1cd&el=desc>
> will *increase* coverage by 0.15%.
> The diff coverage is 100.00%.
>
> [image: Impacted file tree graph]
> <https://codecov.io/gh/miekg/dns/pull/1148?src=pr&el=tree>
>
> @@            Coverage Diff             @@
> ##           master    #1148      +/-   ##
>
> ==========================================
> + Coverage   55.56%   55.71%   +0.15%
>
> ==========================================
>
>   Files          41       41
>
>   Lines       10124    10130       +6
>
> ==========================================
> + Hits         5625     5644      +19
> + Misses       3476     3469       -7
> + Partials     1023     1017       -6
>
> Impacted Files <https://codecov.io/gh/miekg/dns/pull/1148?src=pr&el=tree> Coverage
> Δ
> scan.go
> <https://codecov.io/gh/miekg/dns/pull/1148/diff?src=pr&el=tree#diff-c2Nhbi5nbw==> 84.48%
> <100.00%> (+2.01%) ⬆️
> ------------------------------
>
> Continue to review full report at Codecov
> <https://codecov.io/gh/miekg/dns/pull/1148?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/1148?src=pr&el=footer>. Last update
> 34cecfe...8e1be54
> <https://codecov.io/gh/miekg/dns/pull/1148?src=pr&el=lastupdated>. Read
> the comment docs <https://docs.codecov.io/docs/pull-request-comments>.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/miekg/dns/pull/1148#issuecomment-673135306>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AACWIW3WJ6UH6M5HCGP3YVTSAMHMFANCNFSM4P5LNX5A>
> .
>


Patch:
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/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) {