Skip to content

Commit 8cb0227

Browse files
author
Alvaro Muñoz
committed
include review feedback
1 parent 13242df commit 8cb0227

File tree

4 files changed

+65
-37
lines changed

4 files changed

+65
-37
lines changed

‎go/ql/lib/semmle/go/frameworks/Twirp.qll

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ module Twirp {
4040
* A type representing a protobuf message.
4141
*/
4242
class ProtobufMessageType extends Type {
43-
ProtobufMessage() {
43+
ProtobufMessageType() {
4444
exists(TypeEntity te |
4545
te.getType() = this and
4646
te.getDeclaration().getLocation().getFile() instanceof ProtobufGeneratedFile
@@ -51,34 +51,34 @@ module Twirp {
5151
/**
5252
* An interface type representing a Twirp service.
5353
*/
54-
class ServiceInterface extends InterfaceType {
54+
class ServiceInterfaceType extends InterfaceType {
5555
NamedType namedType;
5656

57-
ServiceInterface() {
57+
ServiceInterfaceType() {
5858
exists(TypeEntity te |
59-
te.getType() = serviceInterface and
60-
serviceInterface.getUnderlyingType() = this and
59+
te.getType() = namedType and
60+
namedType.getUnderlyingType() = this and
6161
te.getDeclaration().getLocation().getFile() instanceof ServicesGeneratedFile
6262
)
6363
}
6464

6565
/**
6666
* Gets the name of the interface.
6767
*/
68-
override string getName() { result = serviceInterface.getName() }
68+
override string getName() { result = namedType.getName() }
6969

7070
/**
7171
* Returns the named type on top of this interface type
7272
*/
73-
NamedType getNamedType() { result = serviceInterface }
73+
NamedType getNamedType() { result = namedType }
7474
}
7575

7676
/**
7777
* A Twirp client.
7878
*/
79-
class ServiceClient extends NamedType {
80-
ServiceClient() {
81-
exists(ServiceInterface i, PointerType p |
79+
class ServiceClientType extends NamedType {
80+
ServiceClientType() {
81+
exists(ServiceInterfaceType i, PointerType p |
8282
p.implements(i) and
8383
this = p.getBaseType() and
8484
this.getName().toLowerCase() = i.getName().toLowerCase() + ["protobuf", "json"] + "client"
@@ -89,9 +89,9 @@ module Twirp {
8989
/**
9090
* A Twirp server
9191
*/
92-
class ServiceServer extends NamedType {
93-
ServiceServer() {
94-
exists(ServiceInterface i |
92+
class ServiceServerType extends NamedType {
93+
ServiceServerType() {
94+
exists(ServiceInterfaceType i |
9595
this.implements(i) and
9696
this.getName().toLowerCase() = i.getName().toLowerCase() + "server"
9797
)
@@ -103,7 +103,7 @@ module Twirp {
103103
*/
104104
class ClientConstructor extends Function {
105105
ClientConstructor() {
106-
exists(ServiceClient c |
106+
exists(ServiceClientType c |
107107
this.getName().toLowerCase() = "new" + c.getName().toLowerCase() and
108108
this.getParameter(0).getType() instanceof StringType and
109109
this.getParameterType(1).getName() = "HTTPClient"
@@ -117,7 +117,7 @@ module Twirp {
117117
*/
118118
class ServerConstructor extends Function {
119119
ServerConstructor() {
120-
exists(ServiceServer c, ServiceInterface i |
120+
exists(ServiceServerType c, ServiceInterfaceType i |
121121
this.getName().toLowerCase() = "new" + c.getName().toLowerCase() and
122122
this.getParameter(0).getType() = i.getNamedType()
123123
)
@@ -145,10 +145,9 @@ module Twirp {
145145
*/
146146
class ServiceHandler extends Method {
147147
ServiceHandler() {
148-
exists(DataFlow::CallNode call, Type handlerType, ServiceInterface i |
148+
exists(DataFlow::CallNode call, Type handlerType, ServiceInterfaceType i |
149149
call.getTarget() instanceof ServerConstructor and
150150
call.getArgument(0).getType() = handlerType and
151-
handlerType.implements(i) and
152151
this = handlerType.getMethod(_) and
153152
this.implements(i.getNamedType().getMethod(_))
154153
)
@@ -164,7 +163,7 @@ module Twirp {
164163
Request() {
165164
handler.getParameter(0).getType().hasQualifiedName("context", "Context") and
166165
handler.getParameter(_) = this.asParameter() and
167-
this.getType().(PointerType).getBaseType() instanceof ProtobufMessage
166+
this.getType().(PointerType).getBaseType() instanceof ProtobufMessageType
168167
}
169168

170169
override predicate isParameterOf(Callable c, int i) {

‎go/ql/test/library-tests/semmle/go/frameworks/Twirp/server/main.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"context"
5+
"strconv"
56
"fmt"
67
"net/http"
78
"time"
@@ -26,6 +27,9 @@ func (s *notesService) CreateNote(ctx context.Context, params *notes.CreateNoteP
2627
CreatedAt: time.Now().UnixMilli(),
2728
}
2829

30+
notes.NewNotesServiceProtobufClient(params.Text, &http.Client{}) // test: ssrfSink, ssrf
31+
notes.NewNotesServiceProtobufClient(strconv.FormatInt(int64(s.CurrentId), 10), &http.Client{}) // test: ssrfSink, !ssrf
32+
2933
s.Notes = append(s.Notes, note)
3034

3135
s.CurrentId++

‎go/ql/test/library-tests/semmle/go/frameworks/Twirp/tests.expected

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,17 @@ passingPositiveTests
55
| PASSED | message | rpc/notes/service.pb.go:86:32:86:49 | comment |
66
| PASSED | message | rpc/notes/service.pb.go:133:33:133:50 | comment |
77
| PASSED | message | rpc/notes/service.pb.go:171:33:171:50 | comment |
8-
| PASSED | request | server/main.go:18:111:18:140 | comment |
9-
| PASSED | request | server/main.go:36:126:36:155 | comment |
8+
| PASSED | request | server/main.go:19:111:19:140 | comment |
9+
| PASSED | request | server/main.go:40:126:40:155 | comment |
1010
| PASSED | serverConstructor | rpc/notes/service.twirp.go:334:81:334:106 | comment |
1111
| PASSED | serviceClient | rpc/notes/service.twirp.go:44:42:44:63 | comment |
1212
| PASSED | serviceClient | rpc/notes/service.twirp.go:183:38:183:59 | comment |
1313
| PASSED | serviceInterface | rpc/notes/service.twirp.go:34:31:34:55 | comment |
1414
| PASSED | serviceServer | rpc/notes/service.twirp.go:322:34:322:55 | comment |
15+
| PASSED | ssrf | server/main.go:30:67:30:89 | comment |
1516
| PASSED | ssrfSink | client/main.go:12:89:12:105 | comment |
17+
| PASSED | ssrfSink | server/main.go:30:67:30:89 | comment |
18+
| PASSED | ssrfSink | server/main.go:31:97:31:120 | comment |
1619
failingPositiveTests
1720
passingNegativeTests
1821
| PASSED | !handler | rpc/notes/service.twirp.go:87:109:87:125 | comment |
@@ -23,5 +26,6 @@ passingNegativeTests
2326
| PASSED | !handler | rpc/notes/service.twirp.go:255:109:255:125 | comment |
2427
| PASSED | !handler | rpc/notes/service.twirp.go:272:120:272:136 | comment |
2528
| PASSED | !handler | rpc/notes/service.twirp.go:301:124:301:140 | comment |
26-
| PASSED | !ssrfSink | server/main.go:56:51:56:68 | comment |
29+
| PASSED | !ssrf | server/main.go:31:97:31:120 | comment |
30+
| PASSED | !ssrfSink | server/main.go:60:51:60:68 | comment |
2731
failingNegativeTests

‎go/ql/test/library-tests/semmle/go/frameworks/Twirp/tests.ql

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import go
22
import semmle.go.frameworks.Twirp
3+
import semmle.go.security.RequestForgery //.dataflow.ReflectedXssQuery as XssConfig
34

45
class InlineTest extends LineComment {
56
string tests;
@@ -49,22 +50,27 @@ query predicate passingPositiveTests(string res, string expectation, InlineTest
4950
exists(RequestForgery::Sink n | t.inNode(n))
5051
or
5152
expectation = "message" and
52-
exists(Twirp::ProtobufMessage n | t.inType(n))
53+
exists(Twirp::ProtobufMessageType n | t.inType(n))
5354
or
5455
expectation = "serviceInterface" and
55-
exists(Twirp::ServiceInterface n | t.inType(n.getNamedType()))
56+
exists(Twirp::ServiceInterfaceType n | t.inType(n.getNamedType()))
5657
or
5758
expectation = "serviceClient" and
58-
exists(Twirp::ServiceClient n | t.inType(n))
59+
exists(Twirp::ServiceClientType n | t.inType(n))
5960
or
6061
expectation = "serviceServer" and
61-
exists(Twirp::ServiceServer n | t.inType(n))
62+
exists(Twirp::ServiceServerType n | t.inType(n))
6263
or
6364
expectation = "clientConstructor" and
6465
exists(Twirp::ClientConstructor n | t.inEntity(n))
6566
or
6667
expectation = "serverConstructor" and
6768
exists(Twirp::ServerConstructor n | t.inEntity(n))
69+
or
70+
expectation = "ssrf" and
71+
exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
72+
cfg.hasFlow(_, sink) and t.inNode(sink)
73+
)
6874
)
6975
}
7076

@@ -82,22 +88,27 @@ query predicate failingPositiveTests(string res, string expectation, InlineTest
8288
not exists(RequestForgery::Sink n | t.inNode(n))
8389
or
8490
expectation = "message" and
85-
not exists(Twirp::ProtobufMessage n | t.inType(n))
91+
not exists(Twirp::ProtobufMessageType n | t.inType(n))
8692
or
8793
expectation = "serviceInterface" and
88-
not exists(Twirp::ServiceInterface n | t.inType(n.getNamedType()))
94+
not exists(Twirp::ServiceInterfaceType n | t.inType(n.getNamedType()))
8995
or
9096
expectation = "serviceClient" and
91-
not exists(Twirp::ServiceClient n | t.inType(n))
97+
not exists(Twirp::ServiceClientType n | t.inType(n))
9298
or
9399
expectation = "serviceServer" and
94-
not exists(Twirp::ServiceServer n | t.inType(n))
100+
not exists(Twirp::ServiceServerType n | t.inType(n))
95101
or
96102
expectation = "clientConstructor" and
97103
not exists(Twirp::ClientConstructor n | t.inEntity(n))
98104
or
99105
expectation = "serverConstructor" and
100106
not exists(Twirp::ServerConstructor n | t.inEntity(n))
107+
or
108+
expectation = "ssrf" and
109+
not exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
110+
cfg.hasFlow(_, sink) and t.inNode(sink)
111+
)
101112
)
102113
}
103114

@@ -115,22 +126,27 @@ query predicate passingNegativeTests(string res, string expectation, InlineTest
115126
not exists(RequestForgery::Sink n | t.inNode(n))
116127
or
117128
expectation = "!message" and
118-
not exists(Twirp::ProtobufMessage n | t.inType(n))
129+
not exists(Twirp::ProtobufMessageType n | t.inType(n))
119130
or
120131
expectation = "!serviceInterface" and
121-
not exists(Twirp::ServiceInterface n | t.inType(n))
132+
not exists(Twirp::ServiceInterfaceType n | t.inType(n))
122133
or
123134
expectation = "!serviceClient" and
124-
not exists(Twirp::ServiceClient n | t.inType(n))
135+
not exists(Twirp::ServiceClientType n | t.inType(n))
125136
or
126137
expectation = "!serviceServer" and
127-
not exists(Twirp::ServiceServer n | t.inType(n))
138+
not exists(Twirp::ServiceServerType n | t.inType(n))
128139
or
129140
expectation = "!clientConstructor" and
130141
not exists(Twirp::ClientConstructor n | t.inEntity(n))
131142
or
132143
expectation = "!serverConstructor" and
133144
not exists(Twirp::ServerConstructor n | t.inEntity(n))
145+
or
146+
expectation = "!ssrf" and
147+
not exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
148+
cfg.hasFlow(_, sink) and t.inNode(sink)
149+
)
134150
)
135151
}
136152

@@ -148,21 +164,26 @@ query predicate failingNegativeTests(string res, string expectation, InlineTest
148164
exists(RequestForgery::Sink n | t.inNode(n))
149165
or
150166
expectation = "!message" and
151-
exists(Twirp::ProtobufMessage n | t.inType(n))
167+
exists(Twirp::ProtobufMessageType n | t.inType(n))
152168
or
153169
expectation = "!serviceInterface" and
154-
exists(Twirp::ServiceInterface n | t.inType(n))
170+
exists(Twirp::ServiceInterfaceType n | t.inType(n))
155171
or
156172
expectation = "!serviceClient" and
157-
exists(Twirp::ServiceClient n | t.inType(n))
173+
exists(Twirp::ServiceClientType n | t.inType(n))
158174
or
159175
expectation = "!serviceServer" and
160-
exists(Twirp::ServiceServer n | t.inType(n))
176+
exists(Twirp::ServiceServerType n | t.inType(n))
161177
or
162178
expectation = "!clientConstructor" and
163179
exists(Twirp::ClientConstructor n | t.inEntity(n))
164180
or
165181
expectation = "!serverConstructor" and
166182
exists(Twirp::ServerConstructor n | t.inEntity(n))
183+
or
184+
expectation = "!ssrf" and
185+
exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
186+
cfg.hasFlow(_, sink) and t.inNode(sink)
187+
)
167188
)
168189
}

0 commit comments

Comments
 (0)