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

SF form injection vs security #20

Closed
mwjames opened this issue Feb 9, 2015 · 6 comments
Closed

SF form injection vs security #20

mwjames opened this issue Feb 9, 2015 · 6 comments
Labels

Comments

@mwjames
Copy link
Contributor

mwjames commented Feb 9, 2015

Creating a user (together with the verification of the input data during the signup) can be highly sensitive and be under special scrutiny for security (email, personal data etc.).

Currently $sfgFormPrinter->formHTML is used the fetch the html generated form definition and the question arises whether this creates a security risk by injecting a form before the data input without verification (false data, stale data etc.).

        list ( $form_text, $javascript_text, $data_text, $form_page_title, $generated_page_name ) =
            $sfgFormPrinter->formHTML( $form_definition, false, false );

Questions

  • Is it OK to allow fields with the property uploadable (or whatever it is called in SF) during a registration process (where a file could contain malicious code)?
  • Is it possible to hijack (via jquery/php from the outside) the form before it is presented to a user on the signup view?
  • Any other question that could potentially be raised with respect to above topic?

@kghbln @toniher @JeroenDeDauw I don't have any answers but I'm looking for some insides whether this can be a real issue or not. In case it is an issue we should at least inform the user base about the potential.

PS: Looking at code samples like below makes me feel uneasy.

        list ( $form_text, $javascript_text, $data_text, $form_page_title, $generated_page_name ) =
            $sfgFormPrinter->formHTML( $form_definition, false, false );

        /* Run hook allow externals to modify output of form */
        wfRunHooks('SemanticSignupPrintForm', array( &$form_text, &$javascript_text, &$data_text, &$form_page_title, &$generated_page_name ) );

        $text = <<<END
                <form name="createbox" id="sfForm" onsubmit="return validate_all()" action="" method="post" class="createbox">
END;
        $text .= $form_text . '</form>';

        $wgOut->addMeta( 'robots', 'noindex,nofollow' );
        $wgOut->addHTML( $text );
@JeroenDeDauw
Copy link
Member

What does this extension use SF for? I thought it stored semantic data, though guess it just stuffs things into a template. If it's just building a template, SF is not needed. I'm guessing the SF form input things are used, and that people can define the registration form using those?

@mwjames
Copy link
Contributor Author

mwjames commented Feb 10, 2015

What does this extension use SF for?

It is just to generate a form and return a template transclusion which maybe better served by just trying to read the template first, generate the fields by adding the appropriate input field definitions without the potential hazard that can be added through some of the SF options.

@mwjames
Copy link
Contributor Author

mwjames commented Feb 10, 2015

Maybe one could assign any page (no need forSF_FORM) to $egSemanticSignupSettings['formName'] that contains a #signupfields parser. #signupfields already generates the standard fields (user name, email using UserFieldsCreateTemplate) therefore extending the parser function could be an option to allow something like:

{{#signupfields:
    |field=Group;input;20
    |field=About me;text;20
    |template=UserSignup
}}

This would isolate html generation to one single source and would allow for strict control of what is being invoked as field before and after the content generation.

@toniher
Copy link
Member

toniher commented Feb 10, 2015

No doubt that the way is presently coded is not very orthodox and it might be potentially risky.
On the other hand, I do think that using SF is a positive point for the extension, so signup forms can use many existing SF features (e. g. values from category).

@kghbln
Copy link
Member

kghbln commented Feb 12, 2015

About files: MW natively checks for the file extension as well as for the mime type, so I believe that there are no worries here.

About SF: + per @toniher

@mwjames
Copy link
Contributor Author

mwjames commented Feb 14, 2015

I have no objections to above comments and just wanted to ensure that at least the question have been raised to avoid any potential issue.

@mwjames mwjames closed this as completed Feb 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants