From f4358173c79d7d73188a0f64a9e1704e74b48412 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 10 Oct 2023 22:34:57 +0200 Subject: [PATCH] limit TitleExtractor to allow only Remark42 whitelisted domains Allowed domains consist of `REMARK_URL` second-level domain (or whole IP in case it's IP like `127.0.0.1`) and `ALLOWED_HOSTS`. That is needed to prevent Remark42 from asking arbitrary servers and storing the page title as the comment.PostTitle. Previous behaviour allowed the caller of the API to create a comment with an arbitrary URL and learn the title of the page, which might be accessible to the server Remark42 is installed on but not to the user outside that network (CWE-918). --- backend/app/cmd/server.go | 39 ++++++++++++++++++++- backend/app/cmd/server_test.go | 21 ++++++++++++ backend/app/rest/api/admin_test.go | 2 +- backend/app/rest/api/rest_public_test.go | 5 +-- backend/app/store/service/service_test.go | 10 +++--- backend/app/store/service/title.go | 42 ++++++++++++++++------- backend/app/store/service/title_test.go | 16 ++++----- 7 files changed, 106 insertions(+), 29 deletions(-) diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index afcfbad4..cfb8236b 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -4,12 +4,14 @@ import ( "context" "embed" "fmt" + "net" "net/http" "net/url" "os" "os/signal" "path" "regexp" + "slices" "strings" "syscall" "time" @@ -504,7 +506,7 @@ func (s *ServerCommand) newServerApp(ctx context.Context) (*serverApp, error) { MaxVotes: s.MaxVotes, PositiveScore: s.PositiveScore, ImageService: imageService, - TitleExtractor: service.NewTitleExtractor(http.Client{Timeout: time.Second * 5}), + TitleExtractor: service.NewTitleExtractor(http.Client{Timeout: time.Second * 5}, s.getAllowedDomains()), RestrictedWordsMatcher: service.NewRestrictedWordsMatcher(service.StaticRestrictedWordsLister{Words: s.RestrictedWords}), } dataService.RestrictSameIPVotes.Enabled = s.RestrictVoteIP @@ -633,6 +635,41 @@ func (s *ServerCommand) newServerApp(ctx context.Context) (*serverApp, error) { }, nil } +// Extract second level domains from s.RemarkURL and s.AllowedHosts. +// It can be and IP like http//127.0.0.1 in which case we need to use whole IP as domain +func (s *ServerCommand) getAllowedDomains() []string { + rawDomains := s.AllowedHosts + rawDomains = append(rawDomains, s.RemarkURL) + allowedDomains := []string{} + for _, rawURL := range rawDomains { + // case of 'self' AllowedHosts, which is not a valid rawURL name + if rawURL == "self" || rawURL == "'self'" || rawURL == "\"self\"" { + continue + } + // AllowedHosts usually don't have https:// prefix + if !strings.HasPrefix(rawURL, "http://") && !strings.HasPrefix(rawURL, "https://") { + rawURL = "https://" + rawURL + } + parsedURL, err := url.Parse(rawURL) + if err != nil { + log.Printf("[WARN] failed to parse URL %s for TitleExtract whitelist: %v", rawURL, err) + continue + } + domain := parsedURL.Hostname() + // if domain is not IP and has more than two levels, extract second level domain + if net.ParseIP(domain) == nil && len(strings.Split(domain, ".")) > 2 { + domain = strings.Join(strings.Split(domain, ".")[len(strings.Split(domain, "."))-2:], ".") + } + + if domain != "" && // don't add empty domain as it will allow everything to be extracted + !slices.Contains(allowedDomains, domain) && // don't duplicate domains + (domain == "localhost" || len(strings.Split(domain, ".")) > 1) { // don't allow single-word domains like "com" except localhost + allowedDomains = append(allowedDomains, domain) + } + } + return allowedDomains +} + // Run all application objects func (a *serverApp) run(ctx context.Context) error { if a.AdminPasswd != "" { diff --git a/backend/app/cmd/server_test.go b/backend/app/cmd/server_test.go index d22213b8..0ad9a9f5 100644 --- a/backend/app/cmd/server_test.go +++ b/backend/app/cmd/server_test.go @@ -726,6 +726,27 @@ func Test_splitAtCommas(t *testing.T) { } } +func Test_getAllowedDomains(t *testing.T) { + tbl := []struct { + s ServerCommand + allowedDomains []string + }{ + // correct example, parsed and returned as allowed domain + {ServerCommand{AllowedHosts: []string{}, CommonOpts: CommonOpts{RemarkURL: "https://remark42.example.org"}}, []string{"example.org"}}, + {ServerCommand{AllowedHosts: []string{}, CommonOpts: CommonOpts{RemarkURL: "http://localhost"}}, []string{"localhost"}}, + // incorrect URLs, so Hostname is empty but returned list doesn't include empty string as it would allow any domain + {ServerCommand{AllowedHosts: []string{}, CommonOpts: CommonOpts{RemarkURL: "bad hostname"}}, []string{}}, + {ServerCommand{AllowedHosts: []string{}, CommonOpts: CommonOpts{RemarkURL: "not_a_hostname"}}, []string{}}, + // test removal of 'self', multiple AllowedHosts and deduplication + {ServerCommand{AllowedHosts: []string{"'self'", "example.org", "test.example.org", "remark42.com"}, CommonOpts: CommonOpts{RemarkURL: "https://example.org"}}, []string{"example.org", "remark42.com"}}, + } + for i, tt := range tbl { + t.Run(strconv.Itoa(i), func(t *testing.T) { + assert.Equal(t, tt.allowedDomains, tt.s.getAllowedDomains()) + }) + } +} + func chooseRandomUnusedPort() (port int) { for i := 0; i < 10; i++ { port = 40000 + int(rand.Int31n(10000)) diff --git a/backend/app/rest/api/admin_test.go b/backend/app/rest/api/admin_test.go index d84dc43d..a4c0283b 100644 --- a/backend/app/rest/api/admin_test.go +++ b/backend/app/rest/api/admin_test.go @@ -111,7 +111,7 @@ func TestAdmin_Title(t *testing.T) { ts, srv, teardown := startupT(t) defer teardown() - srv.DataService.TitleExtractor = service.NewTitleExtractor(http.Client{Timeout: time.Second}) + srv.DataService.TitleExtractor = service.NewTitleExtractor(http.Client{Timeout: time.Second}, []string{"127.0.0.1"}) tss := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.String() == "/post1" { _, err := w.Write([]byte("post1 blah 123 2222")) diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index 65234398..57cdf29e 100644 --- a/backend/app/rest/api/rest_public_test.go +++ b/backend/app/rest/api/rest_public_test.go @@ -480,7 +480,7 @@ func TestRest_FindUserComments(t *testing.T) { func TestRest_FindUserComments_CWE_918(t *testing.T) { ts, srv, teardown := startupT(t) - srv.DataService.TitleExtractor = service.NewTitleExtractor(http.Client{Timeout: time.Second}) // required for extracting the title, bad URL test + srv.DataService.TitleExtractor = service.NewTitleExtractor(http.Client{Timeout: time.Second}, []string{"radio-t.com"}) // required for extracting the title, bad URL test defer srv.DataService.TitleExtractor.Close() defer teardown() @@ -496,7 +496,8 @@ func TestRest_FindUserComments_CWE_918(t *testing.T) { assert.False(t, backendRequestedArbitraryServer) addComment(t, arbitraryURLComment, ts) - assert.True(t, backendRequestedArbitraryServer) + assert.False(t, backendRequestedArbitraryServer, + "no request is expected to the test server as it's not in the list of the allowed domains for the title extractor") res, code := get(t, ts.URL+"/api/v1/comments?site=remark42&user=provider1_dev") assert.Equal(t, http.StatusOK, code) diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go index 74245d93..e86f765a 100644 --- a/backend/app/store/service/service_test.go +++ b/backend/app/store/service/service_test.go @@ -131,7 +131,7 @@ func TestService_CreateFromPartialWithTitle(t *testing.T) { eng, teardown := prepStoreEngine(t) defer teardown() b := DataStore{Engine: eng, AdminStore: ks, - TitleExtractor: NewTitleExtractor(http.Client{Timeout: 5 * time.Second})} + TitleExtractor: NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"})} defer b.Close() postPath := "/post/42" @@ -195,7 +195,7 @@ func TestService_SetTitle(t *testing.T) { eng, teardown := prepStoreEngine(t) defer teardown() b := DataStore{Engine: eng, AdminStore: ks, - TitleExtractor: NewTitleExtractor(http.Client{Timeout: 5 * time.Second})} + TitleExtractor: NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"})} defer b.Close() comment := store.Comment{ Text: "text", @@ -1658,10 +1658,10 @@ func TestService_DoubleClose_Static(t *testing.T) { eng, teardown := prepStoreEngine(t) defer teardown() b := DataStore{Engine: eng, AdminStore: ks, - TitleExtractor: NewTitleExtractor(http.Client{Timeout: 5 * time.Second})} - b.Close() + TitleExtractor: NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{})} + assert.NoError(t, b.Close()) // second call should not result in panic or errors - b.Close() + assert.NoError(t, b.Close()) } // makes new boltdb, put two records diff --git a/backend/app/store/service/title.go b/backend/app/store/service/title.go index f3039390..22c19cc2 100644 --- a/backend/app/store/service/title.go +++ b/backend/app/store/service/title.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "net/http" + "net/url" "strings" "time" @@ -19,14 +20,16 @@ const ( // TitleExtractor gets html title from remote page, cached type TitleExtractor struct { - client http.Client - cache lcw.LoadingCache + client http.Client + cache lcw.LoadingCache + allowedDomains []string } // NewTitleExtractor makes extractor with cache. If memory cache failed, switching to no-cache -func NewTitleExtractor(client http.Client) *TitleExtractor { +func NewTitleExtractor(client http.Client, allowedDomains []string) *TitleExtractor { res := TitleExtractor{ - client: client, + client: client, + allowedDomains: allowedDomains, } var err error res.cache, err = lcw.NewExpirableCache(lcw.TTL(teCacheTTL), lcw.MaxKeySize(teCacheMaxRecs)) @@ -38,13 +41,28 @@ func NewTitleExtractor(client http.Client) *TitleExtractor { } // Get page for url and return title -func (t *TitleExtractor) Get(url string) (string, error) { +func (t *TitleExtractor) Get(pageURL string) (string, error) { + // parse domain of the URL and check if it's in the allowed list + u, err := url.Parse(pageURL) + if err != nil { + return "", fmt.Errorf("failed to parse url %s: %w", pageURL, err) + } + allowed := false + for _, domain := range t.allowedDomains { + if strings.HasSuffix(u.Hostname(), domain) { + allowed = true + break + } + } + if !allowed { + return "", fmt.Errorf("domain %s is not allowed", u.Host) + } client := http.Client{Timeout: t.client.Timeout, Transport: t.client.Transport} defer client.CloseIdleConnections() - b, err := t.cache.Get(url, func() (interface{}, error) { - resp, err := client.Get(url) - if err != nil { - return nil, fmt.Errorf("failed to load page %s: %w", url, err) + b, err := t.cache.Get(pageURL, func() (interface{}, error) { + resp, e := client.Get(pageURL) + if e != nil { + return nil, fmt.Errorf("failed to load page %s: %w", pageURL, e) } defer func() { if err = resp.Body.Close(); err != nil { @@ -52,19 +70,19 @@ func (t *TitleExtractor) Get(url string) (string, error) { } }() if resp.StatusCode != 200 { - return nil, fmt.Errorf("can't load page %s, code %d", url, resp.StatusCode) + return nil, fmt.Errorf("can't load page %s, code %d", pageURL, resp.StatusCode) } title, ok := t.getTitle(resp.Body) if !ok { - return nil, fmt.Errorf("can't get title for %s", url) + return nil, fmt.Errorf("can't get title for %s", pageURL) } return title, nil }) // on error save result (empty string) to cache too and return "" title if err != nil { - _, _ = t.cache.Get(url, func() (interface{}, error) { return "", nil }) + _, _ = t.cache.Get(pageURL, func() (interface{}, error) { return "", nil }) return "", err } diff --git a/backend/app/store/service/title_test.go b/backend/app/store/service/title_test.go index 3e3bd79b..b57d1235 100644 --- a/backend/app/store/service/title_test.go +++ b/backend/app/store/service/title_test.go @@ -28,7 +28,7 @@ func TestTitle_GetTitle(t *testing.T) { {` 2222`, false, ""}, } - ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}) + ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{}) defer ex.Close() for i, tt := range tbl { tt := tt @@ -41,7 +41,7 @@ func TestTitle_GetTitle(t *testing.T) { } func TestTitle_Get(t *testing.T) { - ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}) + ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"}) defer ex.Close() var hits int32 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -75,7 +75,7 @@ func TestTitle_GetConcurrent(t *testing.T) { for n := 0; n < 1000; n++ { body += "something something blah blah\n" } - ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}) + ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"}) defer ex.Close() var hits int32 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -104,7 +104,7 @@ func TestTitle_GetConcurrent(t *testing.T) { } func TestTitle_GetFailed(t *testing.T) { - ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}) + ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"}) defer ex.Close() var hits int32 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -124,9 +124,9 @@ func TestTitle_GetFailed(t *testing.T) { assert.Equal(t, int32(1), atomic.LoadInt32(&hits), "hit once, errors cached") } -func TestTitle_DoubleClosed(*testing.T) { - ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}) - ex.Close() +func TestTitle_DoubleClosed(t *testing.T) { + ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{}) + assert.NoError(t, ex.Close()) // second call should not result in panic - ex.Close() + assert.NoError(t, ex.Close()) }