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

RT not working without MD5 #268

Open
adrienbu opened this issue Aug 9, 2019 · 4 comments
Open

RT not working without MD5 #268

adrienbu opened this issue Aug 9, 2019 · 4 comments
Assignees

Comments

@adrienbu
Copy link

@adrienbu adrienbu commented Aug 9, 2019

When building a BOOMR with the RT plugin, a page_unload call fires a javascript error if the MD5 plugin was not iuncluded as well (l. 1040 of rt.js)
Uncaught TypeError: BOOMR.utils.MD5 is not a function

If MD5 is mandatory for using RT, it should be clearly stated in documentation. If this is not intendend, RT plugin should check if BOOMR.utils.MD5 is defined before using it.

@bluesmoon

This comment has been minimized.

Copy link

@bluesmoon bluesmoon commented Aug 9, 2019

Thanks for the report. We should be checking for the plugin before using it.

@ceckoslab

This comment has been minimized.

Copy link

@ceckoslab ceckoslab commented Sep 6, 2019

Hello @bluesmoon

I looked at this issue and I am considering a solution where we fallback to original string/url if BOOMR.utils.MD5() is not presented.

It should be something like what has been done as a fallback in hashQueryString() in boomerang.js:

hashQueryString: function(url, stripHash) {
...
	if (!BOOMR.utils.MD5) {
		return url;
	}
...
},

Boomerang users should consider that they will have in their beacons the original values of the following beacon parameters:

  • r: URL of page that set the start time of the beacon.
  • nu : URL of next page if the user clicked a link or submitted a form

So far I found 4 references of BOOMR.utils.MD5() in RT plugin and 1 reference in Boomerang library.

@bluesmoon

This comment has been minimized.

Copy link

@bluesmoon bluesmoon commented Sep 6, 2019

There are 2 reasons we MD5 the URLs.

  1. To reduce the beacon size
  2. So that bad security scanning tools don't raise a false XSS alarm thinking that we're injecting url parameters into the page

MD5 is not a secure algorithm, so there is no expectation that the checksum be safe from collisions or anything.

I think this change is fine as long as we document what will happen with and without the plugin.

@ceckoslab

This comment has been minimized.

Copy link

@ceckoslab ceckoslab commented Sep 7, 2019

Thank you @bluesmoon

My plan is to add small wrapper function in RT plugin for BOOMR.utils.MD5(). If MD5 is not presented it will fallback to returning the original URL. Good name candidate is hashString()

I have one concern regarding having clean code because the code will look like:

urlHash = this.hashString(d.URL);
docReferrerHash = this.hashString((d && d.referrer) || "");

On theory urlHash and docReferrerHash will not be always hashed version of a string but actual string. I believe this could be neglected if most of the people are using MD5 plugin.

Side idea (NOT to be implemented with this issue):

I am really interested in this issue and I am wondering how critical is for Boomerang and plugins to use another lighter hash function instead of MD5. Seems that in Boomerang project we use MD5 mostly for URL strings and it we could save some bytes if there is lightweight replacement of MD5. I found interesting ideas in internet and I believe that combined with some extra logic we can get okaysh uniqueness.

I found this checksum function on Stack Overflow: https://stackoverflow.com/a/3276730

function checksum(s)
{
  var chk = 0x12345678;
  var len = s.length;
  for (var i = 0; i < len; i++) {
      chk += (s.charCodeAt(i) * (i + 1));
  }

  return (chk & 0xffffffff).toString(16);
}

I am not sure what is the probability for checksum collisions but we may get some good uniqueness if we append to the checksum string length and number off occurrences first 2-3 most frequent letters in english text: http://pi.math.cornell.edu/~mec/2003-2004/cryptography/subs/frequencies.html

ceckoslab pushed a commit to ceckoslab/boomerang that referenced this issue Sep 9, 2019
… e2e test.
ceckoslab added a commit to ceckoslab/boomerang that referenced this issue Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.