-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
SQL injection attempts killls Etherpad lite #3502
Comments
That's true, indeed. |
Hey guys, just a reminder about responsible disclosure. Posting publicly without giving us chance to pick can be quite dangerous. |
Hum ... actually it has been already disclosed because it is used by some nasty guys. Also, I don't see how to report it in a non public way. The project description don't mention anyway private feedback loop for security issues. How do you think would I had reported it? |
Afaik we have a responsible disclosure policy. I thought it was on etherpad.org and the github readme.
…________________________________
From: François Poulain <notifications@github.com>
Sent: Monday, 21 January 2019 11:16 am
To: ether/etherpad-lite
Cc: John McLear; Comment
Subject: Re: [ether/etherpad-lite] SQL injection attempts killls Etherpad lite (#3502)
Hey guys, just a reminder about responsible disclosure. Posting publicly without giving us chance to pick can be quite dangerous.
Hum ... actually it has been already disclosed because it is used by some nasty guys.
Also, I don't see how to report it in a non public way. The project description don't mention anyway private feedback loop for security issues. How do you think would I had reported it?
-
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#3502 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AANewII6dbcPV2hlcP4uA5c4OHESwQJuks5vFaK4gaJpZM4X1_tz>.
|
It could be a good idea to add some invitation "found a security issue? tell us about it via ...". Before opening this issue I spent few minutes on github's readme and on etherpad.org without finding such an invitation. Also, as a non native english reader, I could have missed the good terms to seek for. |
Noted. Tnx
…________________________________
From: François Poulain <notifications@github.com>
Sent: Monday, 21 January 2019 2:35 pm
To: ether/etherpad-lite
Cc: John McLear; Comment
Subject: Re: [ether/etherpad-lite] SQL injection attempts killls Etherpad lite (#3502)
It could be a good idea to add some invitation "found a security issue? tell us about it via ...". Before opening this issue I spent few minutes on github and on etherpad.org without finding such an invitation. Also, as a non native english reader, I could have missed the good terms to seek for.
-
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#3502 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AANewFZlv6xtuszRP2CCBwWoDHGgTrFwks5vFdGEgaJpZM4X1_tz>.
|
I couldn't find one either, but I did find you requesting a PR to add one #2499. Perhaps consider just creating a https://securitytxt.org/ as a quick fix? |
Offending code is somewhere in here: https://github.com/ether/yajsml/blob/master/server.js#L98 |
Disclaimer: I'm not clever enough to resolve this. I need someone with expertise to help! |
Hey! Doing some looking into this now. First off: to fend off any fears about SQLi: this has nothing to do with SQL injection, as evidenced by this alternative payload that also causes the same crash:
This is a filename length issue in the caching middleware. Currently the cache key is generated as follows:
This causes the cache key length to be controlled by the length of the path. This is a problem when the cache key is used as a filename on disk, as many filesystems limit filename length to 255 characters. This is why there's an To remedy this, I suggest making the cache key a hash of the path instead of the base64 encoded version. This will ensure a fixed-length filename. I'll submit a PR that does this shortly. |
Hi, @tomnomnom, this is clever. It completely makes sense that cache keys are of fixed length. In this way they stay dependent on the content, with practically non existent risk of collisions. I'll have a look tonight. |
Merged the PR, many thanks! A further note about this: from the nodejs docs we may have to consider the possibility of the
Maybe embedded platforms may not have it (e.g.: ARM, MIPS, Raspberry PIs & similar)? I do not know. A description of such scenarios can be found here:
Anyway, the TypeScript guys dealt with this same issue in microsoft/TypeScript#19100, and they resolved it in an elegant way in microsoft/TypeScript@9677b06. If the importing crypto fails at runtime, they replace the hash algorithm the djb2 algorithm, which is way weaker, but works for their case. An example adapted for our case may be: function djb2Hash(data) {
const chars = data.split("").map(str => str.charCodeAt(0));
return `${chars.reduce((prev, curr) => ((prev << 5) + prev) + curr, 5381)}`;
}
console.log(Buffer.from(djb2Hash('This is a 😎 test of the djb2 hash function')).toString('hex'));
// prints 36373536373437333033 I am not asking you to do this, but the djb2 story is fun: see here, and the original mailing list post by Daniel Bernstein from 1991. He was 20 at the time. |
Follow up which uses |
Hi,
On our server we were getting some Etherpad outage. We relied it to a nasty query:
A "minimal" query example:
This provoke an immediate crash:
We are running the 1.7.0 flavor on Debian Stretch with node v6.14.4 and no specific customization.
We reproduced the behavior on two independents Etherpad installation.
The text was updated successfully, but these errors were encountered: