Repository: miekg/dns
Pull request #1149 (2020-08-13 00:38:05)

validate LOC's lat/long field values not to be out of range
The current implementation of LOC RR's (text) parser doesn't perform range check on the latitude and longitude fields, so it can allow values out of the range specified by RFC1876:
```
       d1:     [0 .. 90]            (degrees latitude)
       d2:     [0 .. 180]           (degrees longitude)
       m1, m2: [0 .. 59]            (minutes latitude/longitude)
       s1, s2: [0 .. 59.999]        (seconds latitude/longitude)
```

This PR makes sure that a textual LOC RDATA unexpectedly contains out of range latutide or longitude values.  Note also that it's possible that the resulting 32-bit encoded value is out of range even if degrees, minutes and seconds in the textual representation are all in range.  This PR prevents this type of out-of-range value, too.  These validations help improve interoperability there's a known implementation (BIND) that rejects out-of-range values in the wire format.

Also, while working on it I've noticed one additional glitch: the current implementation could lose the 'cm' part of some large altitude values, e.g., 42849672.95, due to insufficient precision specified for `ParseFloat`.  This PR fixes that issue, too.


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

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

```diff
@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
+ Coverage   55.56%   55.82%   +0.26%     
==========================================
  Files          41       41              
  Lines       10124    10128       +4     
==========================================
+ Hits         5625     5654      +29     
+ Misses       3476     3466      -10     
+ Partials     1023     1008      -15     
```


| [Impacted Files](https://codecov.io/gh/miekg/dns/pull/1149?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scan\_rr.go](https://codecov.io/gh/miekg/dns/pull/1149/diff?src=pr&el=tree#diff-c2Nhbl9yci5nbw==) | `62.36% <85.71%> (+1.54%)` | :arrow_up: |
| [scan.go](https://codecov.io/gh/miekg/dns/pull/1149/diff?src=pr&el=tree#diff-c2Nhbi5nbw==) | `83.57% <100.00%> (+1.10%)` | :arrow_up: |
| [msg.go](https://codecov.io/gh/miekg/dns/pull/1149/diff?src=pr&el=tree#diff-bXNnLmdv) | `76.53% <0.00%> (-0.69%)` | :arrow_down: |
| [types.go](https://codecov.io/gh/miekg/dns/pull/1149/diff?src=pr&el=tree#diff-dHlwZXMuZ28=) | `74.64% <0.00%> (+0.61%)` | :arrow_up: |

------

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


Patch:
diff --git a/scan.go b/scan.go
index e18566fc..2e5ec270 100644
--- a/scan.go
+++ b/scan.go
@@ -1275,6 +1275,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 +1289,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..25574d6b 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,29 @@ 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)
+		}
+	}
 }
 
 func TestParseDS(t *testing.T) {