Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stack Traces shouldn't be shared for server errors #1871

Open
2 tasks done
brunobowden opened this issue Dec 22, 2020 · 2 comments
Open
2 tasks done

Stack Traces shouldn't be shared for server errors #1871

brunobowden opened this issue Dec 22, 2020 · 2 comments
Assignees
@matthewblain
Labels
security Relating to security source:team From active team members
Milestone
v1.1 - Nigeria wi...

Comments

@brunobowden
Copy link
Collaborator

Summary

Internal server - in this case a null pointer - shouldn't be shared outside the server. Though no information leakage seems to be possible at the moment, this should be fixed for the future.

Reproduction Steps

Note the back quote supplied for the jurisdictionType field as an invalid parameter. This should be rejected as invalid.

Beyond that, the more important fix is that any error like, e.g. Null-Pointer-Exception, shouldn't share the stack trace. We should make this optional though, so it could be shared on the development machines and maybe on the hacking server to be helpful for hackers.

curl -i \
  -H 'Content-Type: application/json' \
  -H 'Who-Client-ID: 00000000-0000-0000-0000-000000000000' \
  -H 'Who-Platform: WEB' \
  -X POST 'https://staging.whocoronavirus.org/WhoService/getCaseStats' \
  -d '{ "jurisdictions": [ { "jurisdictionType": "`"} ] }'

HTTP/2 500 
content-type: text/html;charset=utf-8
x-cloud-trace-context: f49bfc0c69eba3b36e1eb6babc64e3ca;o=1
date: Tue, 22 Dec 2020 19:13:22 GMT
server: Google Frontend
content-length: 8916
via: 1.1 google
alt-svc: clear

<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 java.lang.NullPointerException
	at who.StoredCaseStats.load(StoredCaseStats.java:101)
	at who.WhoServiceImpl.getCaseStats(WhoServiceImpl.java:117)
	at sun.reflect.GeneratedMethodAccessor18.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at present.rpc.RpcInvocation$2.proceed(RpcInvocation.java:98)
	at present.rpc.RpcInterceptorChain$ChainedInvocation.proceed(RpcInterceptorChain.java:43)
	at present.rpc.RpcInterceptorChain$ChainedInvocation.proceed(RpcInterceptorChain.java:43)
...

Expected Behavior

  1. That if this error occurs, no stack trace is given
  2. That this causes a 400 bad argument response

Checklist:
@brunobowden brunobowden added security Relating to security source:team From active team members labels Dec 22, 2020
@brunobowden brunobowden added this to the v1.0 - Nigeria milestone Dec 22, 2020
@matthewblain
Copy link
Collaborator

https://github.com/presentco/present-rpc/blob/62b2443649b36a42a2e5f2ab62deb1eee6e73c0f/java/server/src/main/java/present/rpc/RpcFilter.java#L179

@brunobowden
Copy link
Collaborator Author

@crazybob - any chance present-rpc can make it configurable to hide stack traces upon errors?

@matthewblain well spotted

@brunobowden brunobowden changed the title Stack Traces shouldn't be shared for server null pointer exception Stack Traces shouldn't be shared for server errors Dec 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matthewblain matthewblain

Labels
security Relating to security source:team From active team members
Projects
None yet
Milestone
v1.1 - Nigeria with Symptom Tool
Development

No branches or pull requests
2 participants
@brunobowden @matthewblain