Special characters don't get unserialized correctly

[tombouctou]

The problem is simple: unserializeSession splits input into parts using regexps, therefore, if some string data contains |, unserializeSession may fail to parse data. I also got an infinite loop in some cases (cannot get minimal example).

[tombouctou]

I suggest the following parser function (it assumes _unserialize works correctly):

    function unserialize(data) {
        var pos = 0;
        var ret = { };
        do {
            var key = ''; 
            var c = '|';
            while(data.length > pos && (c = data.charAt(pos)) != '|') {
                key += c; pos++;
            }   
            if (key == '' || key == "\r" || key == "\n") break; // eof
            pos++; // skip '|'
            var r = _unserialize(data, pos);
            if (r[1] == 0) return null; // parser stuck
            pos += r[1];
            ret[key] = r[2];
        } while (pos < data.length);
        return ret;
    }

I got it fixed in my fork https://github.com/crowdlab/js-php-unserialize , but I also changed some code structure.

[jarodium]

Hello

I'm having same issue here.

[Alex-Nabu]

serious problem. will try the port

[naholyr]

TODO: check if this port is well merged before next publish

[aalexgabi]

@tombouctou Thank you for the fix. I had this problem when the session data was containing a string which represented a php serialized object.

@naholyr Can you merge this? @tombouctou Can you publish your fork on npm if this doesn't get merged?

[philpoore]

Yeah this caused us MAJOR issues with our api restarting in production.

tl;dr... In the good case, the regexp does just about what it needs to, but in the error case this regexp has catastrophic backtracking.

Example: regex101.com regex: ^((?:.*?[;}])+)([^;}]+?)$ input: a;a;a;a;a;a;a;a;a;a;a;a;a;d;d;d;d;d;

This causes CPU to hit 100% and node v8 to lock up if:

1. There are "|" in strings in the session data.
2. There are "\n" or "\r" in the session data.

Switching to the fork, seems to have solved our problems by replacing the regex with a while loop. npm i https://github.com/crowdlab/js-php-unserialize

BE WARNED: This package as it stands is HIGHLY unreliable and can cause node.js to lock up, only a server restart can bring it back.

[naholyr]

Thanks for this report, I'll see if I can get back to this code and fix it, but I think it's more reasonable to consider this package deprecated. As a workaround I'd advise building a SSO-like between PHP and Node servers (Node receives cookie, requests a dedicated API on PHP with this cookie injected which responds with session's content), or take a look at this article: https://gonzalo123.com/2011/07/25/using-node-js-to-store-php-sessions/

On Mon, 11 Nov 2019 at 11:35, Phil Poore [email protected] wrote:

Yeah this caused us MAJOR issues with our api restarting in production.

tl;dr... In the good case, the regexp does just about what it needs to, but in the error case this regexp has catastrophic backtracking.

Example: regex101.com regex: ^((?:.*?[;}])+)([^;}]+?)$ input: a;a;a;a;a;a;a;a;a;a;a;a;a;d;d;d;d;d;

This causes CPU to hit 100% and node v8 to lock up if:

1. There are "|" in strings in the session data.
2. There are "\n" or "\r" in the session data.

Switching to the fork, seems to have solved our problems by replacing the regex with a while loop. npm i https://github.com/crowdlab/js-php-unserialize

BE WARNED: This package as it stands is HIGHLY unreliable and can cause node.js to lock up, only a server restart can bring it back.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/naholyr/js-php-unserialize/issues/3?email_source=notifications&email_token=AABUIM4ZXAFRRNP2IXZHXU3QTEYOXA5CNFSM4AEMRLYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDWMHOQ#issuecomment-552387514, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUIM76XPYJDOJFMMMLOETQTEYOXANCNFSM4AEMRLYA .

-- Nicolas Chambrier, aka naholyr

Blog : http://naholyr.fr

[naholyr]

@tombouctou are you interested in taking this repository's control? Or anyone who'd like to actively maintain this module? Last time, I failed giving control to an interested user and I highly regret this situation today, so I'd love to fix it.

If noone is interested, I think the most appropriate and responsible move for me is to deprecate the module officially as there is a serious security issue.

[tombouctou]

@naholyr it's better to make it deprecated imo, I currently don't use this module anymore, so I won't take control, sorry.

[philpoore]

Hi @naholyr if the offer is open. I wouldn't mind taking ownership of it. I'm currently using it production, and have no plans to stop using it for at least the next 6 / 12 months.

[huntr-helper]

👋 Hey! We've recently opened a bug bounty against this issue, so if you want to get rewarded 💰 for fixing this vulnerability 🕷, head over to https://huntr.dev!