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.

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

Security Patch #26

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@yewang

yewang commented May 13, 2015

Attempts to patch some security vulnerabilities

@yewang

This comment has been minimized.

Show comment
Hide comment
@yewang

yewang Jun 14, 2015

This pull request is an attempt to address the XSS security vulnerabilities reported here:
#27

yewang commented Jun 14, 2015

This pull request is an attempt to address the XSS security vulnerabilities reported here:
#27

@yewang

This comment has been minimized.

Show comment
Hide comment
@yewang

yewang Jun 14, 2015

The modifications in sgf.js escape special HTML characters ("<", ">", and "&") found in SGF values, disabling the ability to embed HTML in SGF comments, game info, etc.

Removing lines 509-513 in player.js removes the ability to load a JSON file by URL. This feature does not seem to be widely used. If it is desired to keep this feature instead, note that it would be necessary to also escape any special HTML characters found in the comments and game info found in the JSON file, since loading a game record via JSON would bypass the sanitizing steps added to the SGF parser.

The latter two changes to the player.js replaces the unsafe usage of eval with calls to the JSON parser.

Note: it would be much better (but also much more involved) to refactor/redesign EidoGo to avoid overly using innerHTML. The approach used in this patch (of sanitizing SGF input) may mitigate the risk of persistent (stored) XSS attacks, but it does not fix the possibility of reflected XSS attacks that exploit the possibility of malicious code being added to the DOM via the EidoGo interface (i.e., via the ability to set comments/game info from the UI, rather than being loaded through an SGF file).

yewang commented Jun 14, 2015

The modifications in sgf.js escape special HTML characters ("<", ">", and "&") found in SGF values, disabling the ability to embed HTML in SGF comments, game info, etc.

Removing lines 509-513 in player.js removes the ability to load a JSON file by URL. This feature does not seem to be widely used. If it is desired to keep this feature instead, note that it would be necessary to also escape any special HTML characters found in the comments and game info found in the JSON file, since loading a game record via JSON would bypass the sanitizing steps added to the SGF parser.

The latter two changes to the player.js replaces the unsafe usage of eval with calls to the JSON parser.

Note: it would be much better (but also much more involved) to refactor/redesign EidoGo to avoid overly using innerHTML. The approach used in this patch (of sanitizing SGF input) may mitigate the risk of persistent (stored) XSS attacks, but it does not fix the possibility of reflected XSS attacks that exploit the possibility of malicious code being added to the DOM via the EidoGo interface (i.e., via the ability to set comments/game info from the UI, rather than being loaded through an SGF file).

@Xelnath

This comment has been minimized.

Show comment
Hide comment
@Xelnath

Xelnath Mar 13, 2016

Justin - where'd you go?

Xelnath commented Mar 13, 2016

Justin - where'd you go?

@roy7

This comment has been minimized.

Show comment
Hide comment
@roy7

roy7 Nov 25, 2017

Does the pull request also need to make these changes in all.compressed.js?

roy7 commented Nov 25, 2017

Does the pull request also need to make these changes in all.compressed.js?

@yewang

This comment has been minimized.

Show comment
Hide comment
@yewang

yewang Nov 26, 2017

@roy7, this pull request has not made any changes to the compressed version. I left that to the original developer to do, if he accepts these changes.

Unfortunately, the original developer seems to have abandoned this project.

However, several websites have already incorporated the patch and deployed an updated minified file. For example, here is the minified version used by OGS incorporating this patch:
https://cdn.online-go.com/eidogo.min.js

yewang commented Nov 26, 2017

@roy7, this pull request has not made any changes to the compressed version. I left that to the original developer to do, if he accepts these changes.

Unfortunately, the original developer seems to have abandoned this project.

However, several websites have already incorporated the patch and deployed an updated minified file. For example, here is the minified version used by OGS incorporating this patch:
https://cdn.online-go.com/eidogo.min.js

@roy7

This comment has been minimized.

Show comment
Hide comment
@roy7

roy7 Nov 26, 2017

Great, thank you. I updated my application to use the OGS version. :)

roy7 commented Nov 26, 2017

Great, thank you. I updated my application to use the OGS version. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment