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

Encode “<” as \u003c in response #837

Closed
oshev opened this issue Aug 30, 2019 · 7 comments
Closed

Encode “<” as \u003c in response #837

oshev opened this issue Aug 30, 2019 · 7 comments
Assignees
@adriangonz
Milestone
0.5.x

Comments

@oshev
Copy link

oshev commented Aug 30, 2019 •
edited
Loading

Passing "<" as it is in JSON output is potentially dangerous because this opens a possibility to sneak an executable jscript code there, which will be mindless executed by a browser calling prediction API. This can be used for Cross-site scripting (XSS) exploit.

Seldon uses Flask's jsonify library to wrap the response (see source code). Unfortunately, it doesn't encode “<” as \u003c, which is a common mechanism to protect from this problem. It can be tested this way:

from flask import jsonify, Flask
app = Flask(__name__, static_url_path='')
with app.app_context():
	resp = jsonify({"first": "<body onload=alert('test1')>"}
print(resp.data)
# this will output b'{"first":"<body onload=alert(\'test1\')>"}\n'

The output contains an executable JSON (if you copy this JSON to an html file, say a.html and try to open it locally; it'll create an alert in your browser).

The solution is to encode “<” as \u003c in response, either explicitely in Python or using more secure json-convertion libraries.
@ukclivecox
Copy link
Contributor

We should evaluate the recommendations at https://flask.palletsprojects.com/en/1.1.x/security/

@ukclivecox
Copy link
Contributor

However, the engine sits in front of all requests and it is that that returns the final response to the caller. Flask is not exposed directly.

@ukclivecox
Copy link
Contributor

Also, @oshev are you saying that you expect to call Seldon exposed endpoints directly from a client browser? I would assume in production Seldon endpoints would have authentication via an ingress and be accessed from middleware.

@oshev
Copy link
Author

oshev commented Sep 2, 2019

We should evaluate the recommendations at https://flask.palletsprojects.com/en/1.1.x/security/

Yes, it would be great if you evaluate them all!

@oshev
Copy link
Author

oshev commented Sep 2, 2019 •
edited
Loading

However, the engine sits in front of all requests and it is that that returns the final response to the caller. Flask is not exposed directly.

What library do you use in the engine? In couldn't easily check if it encodes “<” as \u003c in the response. To check this, I would need to create a custom Seldon model which returns "<" in the data part of the response.

If you could confirm, that the engine encodes “<” as \u003c in the response, we could close this issue.

@oshev
Copy link
Author

oshev commented Sep 2, 2019

Also, @oshev are you saying that you expect to call Seldon exposed endpoints directly from a client browser? I would assume in production Seldon endpoints would have authentication via an ingress and be accessed from middleware.

If by middleware you mean routing services like Ambassador or Istio, I don't they would modify the Seldon response. In my understanding, they would just pass "<" as it is to the client.

If by middleware you mean some other back-end service in between client and ML, it's possible there won't be one. Some models could be simple / unassuming enough to be called dirrectly from clients browsers. In fact, it's the use case we have. I'll double check, in case I'm missing something.

@ukclivecox ukclivecox added this to the 0.5.x milestone Sep 17, 2019
@adriangonz adriangonz mentioned this issue Sep 27, 2019
Merged
@adriangonz adriangonz self-assigned this Oct 10, 2019
@adriangonz
Copy link
Contributor

Fixed by #893

@adriangonz adriangonz mentioned this issue Apr 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adriangonz adriangonz

Labels
None yet
Projects
None yet
Milestone
0.5.x
Development

No branches or pull requests
3 participants
@adriangonz @oshev @ukclivecox