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.

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

Escape LorisForm value in HTML #9419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maximemulder
Copy link
Contributor

@maximemulder maximemulder commented Oct 23, 2024

The LorisForm class contains an XSS vulnerability as it can paste data from the GET parameters directly into the HTML.

The getValue function reads the GET parameters, but it is also used by code unrelated to the HTML, therefore the escape should be done at the call site, not in the function directly. There may be a risk of double escape as getValue can get its value from other sources.

Having now looked a the code, I agree that we should get rid of LorisForm, building HTML as strings directly (and with so many layers of indirection) is a practice that is 15~20 years old (LORIS is that old so that makes sense !) and unmaintainable IMO.

Testing: Not tested (which page should I try ?).

@maximemulder maximemulder added the Security PR patches a vulnerability, makes resource access changes, or updates dependencies label Oct 23, 2024
@maximemulder maximemulder force-pushed the 2024-10-23_fix-lorisform-escape branch from c34f467 to d2f7d73 Compare October 23, 2024 21:14
@kongtiaowang
Copy link
Contributor

截屏2024-10-24 下午12 56 19 PHP Fatal error: Uncaught TypeError: htmlspecialchars(): Argument #1 ($string) must be of type string, null given in /var/www/Loris/php/libraries/LorisForm.class.inc:949\nStack trace:\n#0 /var/www/Loris/php/libraries/LorisForm.class.inc(949): htmlspecialchars()\n#1 /var/www/Loris/php/libraries/LorisForm.class.inc(1526): LorisForm->textHTML()\n#2 /var/www/Loris/php/libraries/LorisForm.class.inc(1689): LorisForm->renderElement()\n#3 /var/www/Loris/php/libraries/NDB_Page.class.inc(789): LorisForm->toArray()\n#4 /var/www/Loris/php/libraries/NDB_Form.class.inc(139): NDB_Page->display()\n#5 /var/www/Loris/src/Middleware/UserPageDecorationMiddleware.php(248): NDB_Form->handle()\n#6 /var/www/Loris/src/Middleware/PageDecorationMiddleware.php(59): LORIS\\Middleware\\UserPageDecorationMiddleware->process()\n#7 /var/www/Loris/php/libraries/NDB_Page.class.inc(726): LORIS\\Middleware\\PageDecorationMiddleware->process()\n#8 /var/www/Loris/php/libraries/Module.class.inc(322): NDB_Page->process()\n#9 /var/www/Loris/src/Middleware/ResponseGenerator.php(51): Module->handle()\n#10 /var/www/Loris/src/Middleware/AuthMiddleware.php(64): LORIS\\Middleware\\ResponseGenerator->process()\n#11 /var/www/Loris/src/Router/ModuleRouter.php(75): LORIS\\Middleware\\AuthMiddleware->process()\n#12 /var/www/Loris/src/Middleware/ExceptionHandlingMiddleware.php(55): LORIS\\Router\\ModuleRouter->handle()\n#13 /var/www/Loris/src/Router/BaseRouter.php(138): LORIS\\Middleware\\ExceptionHandlingMiddleware->process()\n#14 /var/www/Loris/src/Middleware/ResponseGenerator.php(51): LORIS\\Router\\BaseRouter->handle()\n#15 /var/www/Loris/src/Middleware/ContentLength.php(53): LORIS\\Middleware\\ResponseGenerator->process()\n#16 /var/www/Loris/htdocs/index.php(74): LORIS\\Middleware\\ContentLength->process()\n#17 {main}\n thrown in /var/www/Loris/php/libraries/LorisForm.class.inc on line 949

@maximemulder
Copy link
Contributor Author

Yeah, yeah, I saw the CI errors, haven't had time to tackle them yet no need to remind me 😅 (although I probably should have set the PR as a draft until the errors are resolved, my mistake on that).

@maximemulder maximemulder marked this pull request as draft October 24, 2024 17:24
@maximemulder maximemulder force-pushed the 2024-10-23_fix-lorisform-escape branch from d2f7d73 to 27cfe3b Compare October 24, 2024 19:26
@maximemulder maximemulder marked this pull request as ready for review October 24, 2024 20:31
@ridz1208
Copy link
Collaborator

just adding this for history #6223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Security PR patches a vulnerability, makes resource access changes, or updates dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants