CVE-2023-46998: Document or fix possible XSS vulnerability (via jquery) · Issue #661 · bootboxjs/bootbox

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

Open

JesseDahl opened this issue

May 24, 2018

· 34 comments

Comments

bootbox.confirm and alert use jquery’s .html() (and other functions) that add content to html elements. These are a potential XSS security issue since jquery evaluates the content.

Here’s a working example (scroll down to the bottom of the JS window for the example code, I just borrowed somebody’s fiddle and modified)
https://jsfiddle.net/93sk1zeh/2/

Pass in the following string to the text input field

<script>alert(‘HELLO WORLD’)</script>

it should show 3 separate alert boxes (which verifies it can potentially be used for XSS attacks).

I think there’s two options:

1. Sanitize input before adding it to a DOM element using jquery, or build up the element in a safe manner (i’m not 100% sure the right way to do that just yet tbh)
2. Mention in the documentation the potential danger of passing in user-submitted data as the first parameter to bootbox.confirm() and bootbox.alert(), or, if using an object instead of a string message, as the title property. This way it’s clear the library user is responsible for sanitizing any input that might be used with bootbox.

I suppose I can see your point, although taking input from a user and then (safely) using it in a call to Bootbox as dialog content seems like something that’s the responsibility of the developer, not this library.

I’ll look into whether it’s relatively easy to add something to sanitize message and title content, but I a) really don’t want to write that myself, and b) really dislike adding another dependency to Bootbox.

Yeah I get that.

Maybe just a quick note in the docs about sanitizing your inputs before passing it in to bootbox?

The bootboxjs website is generated from the gh-pages branch, so if you would like to take a crack at that, I’d be happy to review a pull-request. Otherwise, it may be a little while before I get to something like that - what little free-time I have to work on projects like this has been spent trying to get the v5x branch ready for becoming the master branch.

oh yeah, didn’t know the docs were on github too. i’ll take a crack at it.

I’m not sure if this is very hard to fix but I think the current implementation is useful in many cases.
So from a user perspective I want to still be able to show a dialog with HTML elements inside.

From implementation perspective all that needs to be done is to use .text method instead of .html
And I think you can add a constructor called like Bootbox.Unsafe that if provided for message or title will be entered into the DOM using the html method. So unless user wraps the strings in this object explicitly the data will be parsed as text only.

If it sounds reasonable I can probably try to submit a pull request

@yonjah I’m not entirely sure I understand what you’re suggesting.

I won’t be changing the internals to use text() instead of html() - that would break most of the existing functionality, since the message and title options have always allowed HTML.

I’d have to reiterate my previous statement: sanitizing content seems out of scope for this plugin. If I can get 5.0 squared away and pushed to master, maybe I could think about adding an option to allow a sanitizer to be passed in - that would allow devs to sanitize content, without having another large chunk of code for me to maintain.

@tiesont Using text() method by default seem like the best fix to this problem.
But it is indeed a breaking change.

I wouldn’t bother with implementing any solution that can’t be activated by default since users will need to know to activate it. So it’s not much better than changing the documentation and let users decide how they want to handle this issue.

BTW it seem like SweetAlert decided to solve this issue by offering a content property
Which will contain an actual element to be injected into the dialog -
t4t5/sweetalert#845
It’s also have the extra benefit of allowing easier integration with UI frameworks

@vedmant
you just need to make sure you pass your input through a sanitizer before passing it into this library.

@vedmant “fix it” implies that something is broken. I think I’ve made it clear that I don’t consider this a “bug” or problem with Bootbox, in spite of whatever external sites want to claim. If not, well, here it is:

I don’t consider sanitizing the content that gets passed to the message option as in-scope for Bootbox, and it’s been a feature for quite some time to allow HTML to be be passed in, allowing messages shown by bootbox.alert(), bootbox.confirm(), and bootbox.prompt() to have formatted text. It’s not up to bootbox to decide if a programmer has a valid use or not to inject a <script> tag, which in and of itself is not malicious - it’s whatever code it contains and what the intent was that matters.

@JesseDahl @tiesont I don’t have security concerns here, the issue is that Snyk is reporting this package as not safe. Which required to manually add it to Snyk ignore list on every project, which is also a bad idea as if there any real security issue then it also will be ignored and not reported.

This gridlock needs to be resolved in some way, I enjoy using this package safely but cannot get a clean build without getting dirty :(

@nullivex I hope this doesn’t come across poorly, as there’s no malicious intent behind it, but I’m not terribly concerned about what external sites report about this library. Bootbox works the way it does by intent, allowing users to have formatted text in their messages (which is no different than normal Bootstrap modals). I’m not going to change the default behavior and force users to enable HTML via an option, which is probably what I would have to do to make HackerOne et al happy.

It’s always an option to fork this library, make that change, and create a new npm package, if someone really wanted to use Bootbox without warnings, but I’m not interested in doing so at the moment. Granted, I’m not the owner of this package, but since @makeusabrew hasn’t been seen in a while, the most active collaborator (which seems to be me) pretty much becomes the de facto decision maker.

As a note, there is a recent fork that possibly has fixed some of these issues, found here: https://github.com/lddubeau/bootprompt - you may want to investigate if that works better. At the least, no one has filed a vulnerability report on that package yet, so you wouldn’t get the warnings you’re seeing here.

@tiesont Thanks for getting back to me. I appreciate the time you took and sorry to bother you in the first place. I understand that life was all good until a security researcher decided something arbitrarily bad could happen when this library is used improperly. (Typically my thumb hurts after hitting it with a hammer so I think you are completely in the right!) Rather than continue to prod you over this and try to push you a direction you would rather not go. I would like to ask what you would see as a fitting solution to this issue? It could be important for other projects. I think you have ran into a reasonable disagreement with the security research done. I agree with your stance and wish the advisory could be adjusted personally. That being said. I would not mind hearing your opinion.

I hope this finds you well!

@nullivex I wouldn’t call it “bothering” me. I understand that having that warning on the package is a problem for some users, but there’s not a whole lot I can do about it. Someone decided that this small(ish) library needs to act the same way as a large framework like React, so now it’s seen an issue.

As I noted, using bootprompt might a option - it’s Bootbox ported to TypeScript, plus whatever changes that author has made since then. I did a quick scan, and it doesn’t seem to use the html() function, which seems to be what all the fuss is about.

There are also other packages that have ported Bootbox to be jQuery-free, so that it can used in frameworks like Angular (I think one was called ng-bootbox, but it’s been a bit since I last looked), so if you’re using something like React or Angular, you might want to investigate those options.

I have thought about looking into what Bootstrap uses internally, since I think they have some form of HTML sanitizing built into their JavaScript components, if I recall correctly. I know that I don’t have the requisite knowledge to build my own sanitizer, and there isn’t much of a point of introducing an external dependency (developers can do that themselves, after all) if I were to rely on some other library (other than Bootstrap) to sanitize content.

Bootstrap’s sanitizer functions (https://github.com/twbs/bootstrap/blob/master/js/src/util/sanitizer.js) seem fairly straight-forward, but none of these functions are part of it’s public API. I suppose I could pull the code from that file into this project, but that would require manually updating the functions whenever Bootstrap fixes something. I’ll consider it, but not promising anything.

@tarlepp Thoughts?

Using that bootstrap sanitizer would be nice addition, although using it won’t solve all possible attacks with inputs (eg. SQL injections, etc.) - We cannot know how those input data are used in backend side and that isn’t something that bootstrap/bootbox itself should do.

I’ve pinned this issue, to make it easier to find (and hopefully reduce duplicate issues being raised).

And imho this issue is not solved yet, so it should not be closed one - there is some improvements that could be done to bootbox side for this.

it won’t solve all possible attacks with inputs (eg. SQL injections, etc.)

@tarlepp could you elaborate on this point
As far as I know bootbox only injects input into the DOM so it make sense to handle input in a safe manner preventing XSS and DOM related injections.
Does bootbox send SQL queries with inputs? if not how can it cause SQL injections ?

BTW as implemented in #699 the fix desn’t require any sanitation and allows to keep the exact same functionality with a minor API change

@yonjah Minor API change, yes. Major change in functionality, also yes, and I’m not okay with that.

@tarlepp was just making a point that user input should never be trusted implicitly - Bootbox doesn’t do anything that involves back-end code.

@tiesont what is the major functionality change introduced in #699 ?

@tarlepp was just making a point that user input should never be trusted implicitly - Bootbox doesn’t do anything that involves back-end code.

So if it only handles the DOM I would say this is the only kind of injections we should worry about.

@yonjah Making HTML rendering (for the various options which currently allow it) require an extra step, rather than the default behavior, is a major difference, IMO, which is the majority of what your pull-request does. Anyone who’s okay with adding something like

as their message option should be capable of using

$(‘<b>Hello World!</b>’).sanitize() // assuming some external sanitizing library or function

@tiesont so from functionality perspective there is no issue the only change is API change -

bootbox.alert(‘<b>Hello World!</b>’); //Old API bootbox.alert($(‘<b>Hello World!</b>’)); //new API

Functionality is identical in both cases.

I agree that from API change adding some sanitize call when you need the HTML functionality might not be an issue but it probably wont solve the security issue. It will introduce the exact functionality change you are trying to prevent and when you don’t need it it can be a major pain -

bootbox.alert(`Hello ${user.name}`); bootbox.alert(request.error);

This examples are safe to use after the API change but are completely vulnerable with the current API.
if the API expect a call to some sanitize function before passing the input in it will still be vulnerable.

@yonjah I think we’ll have to agree to disagree. I respect your input and value the time you’ve put into this, but I don’t agree with the changes you want to make, so I won’t be pulling them in.

Hi all,

Forgive me if this is a noob question, but doesn’t dev tools give the ability to execute arbitrary javascript code on a page in the same way that this bootbox “vulnerability” gives? Sure, maybe a malicious user can inject code into the bootbox callback, but then it’s still running frontend code which we already know is manipulable by the user.

If this ability to execute code gives malicious users a route to cause arbitrary code to be executed for anyone other than themselves, then I definitely want to fill in the gaps in my understanding.

Ryan

It could be a “stored XSS” type of attack. Some malicious user enters some value that gets stored in the backend. Then later some other user accesses some record or piece of information, and that stored bit of xss payload gets loaded into the app and into bootbox somehow.

In this case the user input is indirect and stored on the backend first before becoming a problem.

Is there any update on the topic? My pipeline warns about this XSS issue and it’s not fixable for me. If it’s not an issue, maybe this can be sorted out with the security repo to remove the warning there?

@GitRon It’s still an issue in that the notice isn’t going to go away unless we change the internals of Bootbox to not use jQuery’s .html() function or introduce an internal sanitizer. I’m not doing either anytime soon, as mentioned above and in related issues.

If you’re never going to use HTML in your messages or titles, you can fork this repo and replace any uses of .html() with respect to the message and title options, which would probably let your fork pass whatever tool was used to determine Bootbox is vulnerable to injection.

This was referenced

Sep 6, 2021

@tiesont, what about having bootbox accept a user provider sanitizer function, like prototyped in https://github.com/makeusabrew/bootbox/compare/master…gberaudo:accept_global_sanitizer?expand=1. This would:

@gberaudo That’s been suggested before, but doesn’t really solve the problem, at least as far as these “scan this project for issues” sites are concerned. There’s nothing stopping anyone from running user input from an external sanitizer already (which seems like a completely rational solution). I suppose it would be a little less work from a developer’s standpoint - supply an sanitizer, then all future values are cleaned automatically. But then again, that’s solvable with a global helper function, so…

The core of the issue is that there seems to be an expectation that our small library, which has been around a while and was never built to be used in the context of things like React, should provide the same sort of “protect me from myself” functionality that much larger libraries do. I don’t have the time or inclination to support an embedded sanitizing function, nor does @tarlepp. Since we’re pretty much it as far as active collaborators, well, you get what we’re okay with supporting.

If we ever decide to do a Bootbox 6, sure, I’ll make sure that there’s a way to define a sanitizer (and provide some sort of basic cleaning), but that’s a project for next year, maybe…

Thanks for your reply @tiesont. I have double checked the discussion in this thread and I did not see where a similar proposition has been made before. Maybe it was proposed in another place? If you have a pointer to it, I would appreciate it because I really like to understand the situation here.

So far, I get that you want:

• no breaking changes (which is fair);
• no undue maintenance burden (writing an internal sanitizer function is a no-go).
  Also you don’t care about bootbox being flagged as “unsafe” by companies like Snyk.

I am completly aligned with what you wrote, it is fair and reasonable to me. That is why I proposed a solution that has the merits of:

• closing this issue;
• being easy to implement and maintain;
• being opt-in, no breaking change;
• being flexible (use or not a sanitizer function, choose the one you like, implement your own, …);
• addressing the problem of unsafe usage of the lib: as soon as a sanitizer is defined, all subsequent usages of bootbox will be secure which is a great step forwards for me and my team, making us confident to press the "I know what I am doing button and whitelist this library".

That is as far as I can go to try to explain and convince you that the current issue is addressable upstream in a sane way.
Thanks for the great discussions and have a good day.