Skip to content

issue #15034 half fix#15041

Merged
88250 merged 1 commit intosiyuan-note:devfrom
syr1ne:dev
Jun 16, 2025
Merged

issue #15034 half fix#15041
88250 merged 1 commit intosiyuan-note:devfrom
syr1ne:dev

Conversation

@syr1ne
Copy link
Copy Markdown
Contributor

@syr1ne syr1ne commented Jun 16, 2025

bug

its the half fix for #15034
the code is renaming and sanitizing the XSS payload named emoji files.
everything works fine, the only thing left is this fix should be executed before the ImportData function in kernel/model/import.go or before emoji is loaded on the frontend otherwise, the XSS still execute whenever someone imports the zip file.

@88250 88250 merged commit 724cddf into siyuan-note:dev Jun 16, 2025
1 check passed
@88250
Copy link
Copy Markdown
Member

88250 commented Jun 16, 2025

感谢你的贡献,思源有你更精彩!
Thank you for your contribution. SiYuan will be more wonderful with you!

88250 added a commit that referenced this pull request Jun 16, 2025
88250 added a commit that referenced this pull request Jun 16, 2025
@88250
Copy link
Copy Markdown
Member

88250 commented Jun 16, 2025

Updated some logic and added checks in ImportData, thanks.

@octodi
Copy link
Copy Markdown

octodi commented Jun 16, 2025

Renaming the file is now working perfectly, but as syr1ne mentioned in his PR renaming should be done before the ImportData function in kernel/model/import.go loads or before emoji is loaded on electron app.

Screen.Recording.2025-06-16.at.8.50.58.PM.mov

From logs:

...
I 2025/06/16 20:51:07 import.go:139: import data [name=xss-space.zip, size=40934]
I 2025/06/16 20:51:07 import.go:662: import data from [/Users/octodi/SiYuan/temp/import/20250616205107.zip]
W 2025/06/16 20:51:07 import.go:697: renaming invalid custom emoji file [" onerror=alert(1) src=".png] to [/Users/octodi/SiYuan/temp/import/20250616205107/data-20250616095415/emojis/quot onerroralert1 srcquot.png]
I 2025/06/16 20:51:07 import.go:710: import data from [/Users/octodi/SiYuan/temp/import/20250616205107.zip] done
...

You could test by importing this data: xss-space.zip

@88250
Copy link
Copy Markdown
Member

88250 commented Jun 17, 2025

image

Found the problem. This is because the icon set previously already has an XSS problem. It will be fixed later.

88250 added a commit that referenced this pull request Jun 17, 2025
@syr1ne
Copy link
Copy Markdown
Contributor Author

syr1ne commented Jun 17, 2025

nice work @88250
i have just found another XSS here:
image

i have figured something out. renaming file is good, we dont need to change that. but it will be better we can just sanitize the conf.json because whatever shows on the electron app will also be in the conf.json
image
so the box.go check is good and its working. just put it on some kind of loop that whenever the conf.json is updated, it will check for a malicious filename and sanitize it automatically. using that, possibly every single XSS can be mitigated.

@octodi
Copy link
Copy Markdown

octodi commented Jun 17, 2025

I think we need to apply FilterUploadFileName() at multiple levels,

image

Also here
image

And I think here also
image

And everywhere where documents are rendered

88250 added a commit that referenced this pull request Jun 17, 2025
@88250
Copy link
Copy Markdown
Member

88250 commented Jun 17, 2025

Added checks to initialize Conf and set notebook icon, please review.

@syr1ne
Copy link
Copy Markdown
Contributor Author

syr1ne commented Jun 17, 2025

the XSS still works.
in the conf.json file, you just now need to filter docIcon
image
in similar way you did here

	for i, emoji := range Conf.Editor.Emoji {
		if strings.Contains(emoji, ".") {
			// XSS through emoji name https://github.com/siyuan-note/siyuan/issues/15034
			emoji = util.FilterUploadFileName(emoji)
			Conf.Editor.Emoji[i] = emoji
		}
	}

i guess that's all what is left and will fully patch the vulnerability.

88250 added a commit that referenced this pull request Jun 17, 2025
@88250
Copy link
Copy Markdown
Member

88250 commented Jun 17, 2025

Thank you very much, please test and verify.

@syr1ne
Copy link
Copy Markdown
Contributor Author

syr1ne commented Jun 17, 2025

XSS is still working. i just figured out that the siyuan in not loading ./conf/conf.json rather it loads the session files for example: ./data/20250615145655-t6o7gej/.siyuan/conf.json
image
i manually changed the icon value in those files and yes, XSS got fixed. so hopefully sanitizing the session files also will fix the issue.

@octodi
Copy link
Copy Markdown

octodi commented Jun 17, 2025

For me changing emoji name in Document properties worked ./data/20250617125532-i8qpgrv/20250617125535-nvysv69.sy and ./data/20250617125532-i8qpgrv/20250617125535-nvysv69/20250617125540-w2btlp5.sy

% cat ./data/20250617125532-i8qpgrv/20250617125535-nvysv69.sy
{"ID":"20250617125535-nvysv69","Spec":"1","Type":"NodeDocument","Properties":{"icon":"changed.png","id":"20250617125535-nvysv69","title":"subdoc","type":"doc","updated":"20250617125538"},"Children":[{"ID":"20250617125535-gbr9xnz","Type":"NodeParagraph","Properties":{"id":"20250617125535-gbr9xnz","updated":"20250617125535"}}]}

@octodi
Copy link
Copy Markdown

octodi commented Jun 17, 2025

I think we should perform sanitization on UI side instead rename file or filtering them on backend

88250 added a commit that referenced this pull request Jun 17, 2025
@88250
Copy link
Copy Markdown
Member

88250 commented Jun 17, 2025

Thank you, thank you, I have revised it again, please review it.

I think we should perform sanitization on UI side instead rename file or filtering them on backend

The front-end UI is used in too many places, so it is more convenient to process it on the back-end.

@octodi
Copy link
Copy Markdown

octodi commented Jun 17, 2025

Changes works now, Thanks ❤️
@syr1ne Please review it

@syr1ne
Copy link
Copy Markdown
Contributor Author

syr1ne commented Jun 17, 2025

@octodi yes, i confirm the fix works perfectly without any issue.
good work @88250

@88250
Copy link
Copy Markdown
Member

88250 commented Jun 18, 2025

Thank you both!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants