[ZEPPELIN-6142] Apply synchronizer token pattern to terminal interpreter for CSWSH protection #4892
Conversation
Apply synchronizer token pattern to terminal interpreter for CSWSH protection
seung-00
changed the title
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(TerminalSocket.class); | ||
|
|
||
| private final Map<String, String> csrfTokens = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
CSRF tokens are managed per note and paragraph.
|
|
||
| // We look for a file, as ClassLoader.getResource() is not | ||
| // designed to look for directories (we resolve the directory later) | ||
| ClassLoader clazz = TerminalThread.class.getClassLoader(); | ||
| URL url = clazz.getResource("html"); | ||
| URL url = clazz.getResource("static"); |
There was a problem hiding this comment.
Renamed html directory to static, as index.html is now rendered by Jinja and has been moved to an ui_templates directory.
| HttpServletRequest request, | ||
| HttpServletResponse response | ||
| ) | ||
| throws ServletException, IOException { |
There was a problem hiding this comment.
Is it OK to remove ServletException here? My IDE shows that it is never thrown in this method.
There was a problem hiding this comment.
I agree with you. That said, other classes that extend from HttpServlet, like PrometheusServlet and IndexHtmlServlet, don’t use these exceptions but keep them explicitly.
IMO, for consistency, it’s better to keep them in this PR too. If you plan to remove them, it’d be better to handle that in a separate PR.
| String csrfToken = TerminalCsrfTokenManager.getInstance().generateToken(noteId, paragraphId); | ||
|
|
||
| Map<String, Object> context = new HashMap<>(); | ||
| context.put("CSRF_TOKEN", csrfToken); |
There was a problem hiding this comment.
From my understanding, this handler is publicly accesible. When testing in my local environment, I noticed that, since it includes the CSRF token, anyone who knows the mapped address for terminal-ui could potentially access the terminal.
It might be worth considering receiving the CSRF token exclusively through the channel between NotebookServer and the WebSocket (e.g., within HTML elements rendered by terminal-dashboard.jinja).
This way, even if someone discovers the terminal UI address, they wouldn't have direct access to the CSRF token.
There was a problem hiding this comment.
You're right. It lacks authentication, so it’s still vulnerable to token theft. But other HTML elements, like terminal-dashboard, aren’t accessible within the iframe for terminal-ui.
|
@tbonelee |
What is this PR for?
This PR applies synchronizer token pattern to terminal interpreter for CSWSH protection. The terminal interpreter is reported with a CSWSH(Cross-Site WebSocket Hijacking) vulnerability. An earlier PR partially addressed this issue by implementing an origin check. However, this measure does not provide full protection, as it can't prevent attacks from scripts running within the same origin.
To enhance security, I implemented the synchronizer token pattern as an additional layer of protection. I modified the terminal HTML to be rendered with a Jinja template, allowing a CSRF token to be injected. This CSRF token is then validated on receiving the first WebSocket message, providing robust protection against CSWSH attacks.
References:
What type of PR is it?
Improvement
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6142
How should this be tested?
Screenshots (if appropriate)
Questions: