[trafficcontrol] branch master updated: Fix nil pointer dereference in PUT /deliveryserivces/{id} endpoint

[ro...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rob pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git


The following commit(s) were added to refs/heads/master by this push:
     new e3867dc  Fix nil pointer dereference in PUT /deliveryserivces/{id} endpoint
e3867dc is described below

commit e3867dc77287e383727b2375b235724ddf9acbae
Author: Rawlin Peters <ra...@comcast.com>
AuthorDate: Thu Jan 17 12:46:28 2019 -0700

    Fix nil pointer dereference in PUT /deliveryserivces/{id} endpoint
    
    If this endpoint receives a json request without a "matchList" array
    (which is not a required field), the endpoint will panic on a nil
    pointer dereference.
    
    Load the delivery service matchList from the DB before dereferencing it.
    As a side effect, this also prevents a user from passing in their own
    "matchList" array which would potentially cause the wrong hostname to be
    updated into the delivery service's ssl keys.
---
 traffic_ops/testing/api/v14/deliveryservices_test.go   |  3 ++-
 .../deliveryservice/deliveryservicesv13.go             | 18 +++++++++---------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/traffic_ops/testing/api/v14/deliveryservices_test.go b/traffic_ops/testing/api/v14/deliveryservices_test.go
index f80fe90..d947878 100644
--- a/traffic_ops/testing/api/v14/deliveryservices_test.go
+++ b/traffic_ops/testing/api/v14/deliveryservices_test.go
@@ -17,10 +17,10 @@ package v14
 
 import (
 	"strconv"
+	"testing"
 
 	"github.com/apache/trafficcontrol/lib/go-log"
 	"github.com/apache/trafficcontrol/lib/go-tc"
-	"testing"
 )
 
 func TestDeliveryServices(t *testing.T) {
@@ -101,6 +101,7 @@ func UpdateTestDeliveryServices(t *testing.T) {
 	updatedMaxDNSAnswers := 164598
 	remoteDS.LongDesc = updatedLongDesc
 	remoteDS.MaxDNSAnswers = updatedMaxDNSAnswers
+	remoteDS.MatchList = nil // verify that this field is optional in a PUT request, doesn't cause nil dereference panic
 
 	if updateResp, err := TOSession.UpdateDeliveryService(strconv.Itoa(remoteDS.ID), &remoteDS); err != nil {
 		t.Errorf("cannot UPDATE DeliveryService by ID: %v - %v\n", err, updateResp)
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservicesv13.go b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservicesv13.go
index 1bfdc16..223013b 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservicesv13.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservicesv13.go
@@ -487,15 +487,6 @@ func update(tx *sql.Tx, cfg config.Config, user *auth.CurrentUser, ds *tc.Delive
 		return tc.DeliveryServiceNullable{}, http.StatusInternalServerError, nil, errors.New("getting CDN domain after update: " + err.Error())
 	}
 
-	// newHostName will be used to determine if SSL Keys need updating - this will be empty if the DS doesn't have SSL keys, because DS types without SSL keys may not have regexes, and thus will fail to get a host name.
-	newHostName := ""
-	if dsType.HasSSLKeys() {
-		newHostName, err = getHostName(ds.Protocol, *ds.Type, *ds.RoutingName, *ds.MatchList, cdnDomain)
-		if err != nil {
-			return tc.DeliveryServiceNullable{}, http.StatusInternalServerError, nil, errors.New("getting hostname after update: " + err.Error())
-		}
-	}
-
 	matchLists, err := GetDeliveryServicesMatchLists([]string{*ds.XMLID}, tx)
 	if err != nil {
 		return tc.DeliveryServiceNullable{}, http.StatusInternalServerError, nil, errors.New("getting matchlists after update: " + err.Error())
@@ -506,6 +497,15 @@ func update(tx *sql.Tx, cfg config.Config, user *auth.CurrentUser, ds *tc.Delive
 		ds.MatchList = &ml
 	}
 
+	// newHostName will be used to determine if SSL Keys need updating - this will be empty if the DS doesn't have SSL keys, because DS types without SSL keys may not have regexes, and thus will fail to get a host name.
+	newHostName := ""
+	if dsType.HasSSLKeys() {
+		newHostName, err = getHostName(ds.Protocol, *ds.Type, *ds.RoutingName, *ds.MatchList, cdnDomain)
+		if err != nil {
+			return tc.DeliveryServiceNullable{}, http.StatusInternalServerError, nil, errors.New("getting hostname after update: " + err.Error())
+		}
+	}
+
 	if newDSType.HasSSLKeys() && oldHostName != newHostName {
 		if err := updateSSLKeys(ds, newHostName, tx, cfg); err != nil {
 			return tc.DeliveryServiceNullable{}, http.StatusInternalServerError, nil, errors.New("updating delivery service " + *ds.XMLID + ": updating SSL keys: " + err.Error())