Sanitization of Fields input

Support General Sanitization of Fields inputResolved

Viewing 15 posts - 1 through 15 (of 30 total)
  • Author
    Posts
  • #15591
    Rao AbidRao Abid
    Participant

    Hi Support,

    I see that the plugin is not sanitizing user input as per field type.

    For instance, text field should be sanitized using the WordPress built-in function sanitize_text_field() function.
    Link: https://developer.wordpress.org/reference/functions/sanitize_text_field/

    Here is the Recordit Screencast for what i am trying to show:
    https://recordit.co/wX2MWS8yjE

    Notice that if a user enters a script, it gets saved to Database as-is without going through text field sanitization.
    I understand not every developer want to run it through this method. Is there a way for me to run the input through these sanitization methods?

    something like this where we can add the sanitization method to use as parameter in field array config:

    https://github.com/boospot/boo-settings-helper/wiki/Fields-Configuration
    Notice the sanitize_callback array parameter.
    Its usage is defined here:
    https://github.com/boospot/boo-settings-helper/wiki/Fields-Configuration#sanitize_callback

    Similar goes with other field types.

    Let me know if you need more clarification on this.

    #15593
    david.hdavid.h
    Participant

    If correct, this is quite concerning.

    Standard WordPress security should be enabled by default in any plugin or snippet that uses the ecosystem - it should be possible to override this with an option but this should not be necessary in normal circumstances.

    Anh, please can you confirm whether this is isolated or across the board.

    #15596
    Anh TranAnh Tran
    Keymaster

    Hi Rao and David,

    The plugin sanitize values for some field types only (file_input, email, url, oembed, checkbox and switch). You can see the code for that here. For other fields, to let users able to enter some HTML, we don't force a sanitize callback. But you can sanitize the value with this code:

    add_filter( 'rwmb_text_sanitize', 'sanitize_text_field' );
    
    add_filter( 'rwmb_{$field_type}_sanitize', 'your_sanitize_callback' );
    #15599
    david.hdavid.h
    Participant

    Hi Anh,

    As firewalls do, you cannot go wrong implementing a default deny policy when coding. Thus, unless you specifically allow something, you deny it - like whitelisting.

    The minimum santization you should incorporate for fields which might contain HTML is KSES:

    https://codex.wordpress.org/Function_Reference/wp_kses
    https://codex.wordpress.org/Function_Reference/wp_kses_post
    https://codex.wordpress.org/Plugin_API/Filter_Reference/pre_kses

    Additionally, create separate allowed HTML tag arrays for back-end admins (more open) and front-end users (less open) which can be amended as per requirements.

    #15601
    Anh TranAnh Tran
    Keymaster

    Hi David,

    Making a whitelist of HTML tags might be tricky, since there are a lot of them. For example, if you have a textarea/wysiwyg field, then it nearly impossible to define those tags. Besides, I want to offer flexibility to developers and let them decide what need to be sanitized.

    I'll add sanitize_callback parameter to the field settings. So developers can decide what and how to sanitize value.

    #15602
    david.hdavid.h
    Participant

    Ok,

    But I'm not sure most of your customers will know what they need to do - especially those using MB User Profile and MB FrontEnd Submission where (potentially) anonymous or less trusted users could abuse the system.

    I understand your concern regarding managing a whitelist for HTML tags/attributes, but wp_kses_post should handle this without any further configuration required:

    https://codex.wordpress.org/Function_Reference/wp_kses_post

    Alternatively, at least baseline strip javascript tags/code and links containing javascript as these should never be included in post markup, and can used to create XXS attacks which maliciously affect users browsing the site.

    #15603
    david.hdavid.h
    Participant

    Oops: XSS attack, not XXS attack.

    #15604
    Anh TranAnh Tran
    Keymaster

    I understand. However, there are some cases where people really want to enter script, like a textarea field for entering header / footer code (in a page settings).

    I just want to provide the maximum flexibility to users, while developers still can restrict the content if they want. That's how a library should do. It's the same as the Customizer API where developers still have to define their own sanitize callback.

    #15639
    Anh TranAnh Tran
    Keymaster

    I've added support for sanitize_callback here. Please try it and let me know if you need any improvement.

    #15656
    Rao AbidRao Abid
    Participant

    Hi Anh,

    I have not tested it yet, but in my opinion, fields should be sanitized out of the box.

    Each field should be sanitized according to its type.

    If some developer do not want to use default sanitization and want to allow script tags or other similar dangerous input, he should do it by explicitly explaining that he dont want to use default sanitization.

    He may use this sanitization_callback in that case.

    I see that your plugin no only skipping sanitization but also not validating input.

    e.g. select field input does not check if the provided value is one of the value of options.
    (Let me know if its doing so)

    I had purchased the bundle before and wanted to use this in my plugin now, but i am not getting comfortable with this practice.

    I know you have two problems if you apply the default sanitization in your next version:

    Problem 1: Current developers may be using the fields knowing that tags are not stripped off. If you apply default sanitization, their plugins may not behave as expected as all tags will be sanitized.

    Problem 2: It may be extra coding hours of debugging and checking.

    Grievance: If you do not apply default sanitization, then sites are at risk of XSS at minimum.

    Solution: You plan it for the major release and let your developers know that fields will be sanitized automatically according to their type, If they dont want to use default sanitization, they have two options:

    1. they can write no_sanitize for the sanitize_callback value and if such value found, you may simple return the value here: https://github.com/wpmetabox/meta-box/blob/e0aa9753a74603fc4a6348f4638c324e48f196f7/inc/sanitizer.php#L51
    2. Or, they can provide their own sanitization function in sanitize_callback

    In any case, there has to be some default sanitization at all cost.
    as @david.h said, it should be white-list approach as compared to blaclist approach.

    Let me know your thoughts and plan on this so that i may decide whether to use this tool in my plugin or not.

    Regards,

    Rao Abid

    #15663
    Anh TranAnh Tran
    Keymaster

    Hi Rao,

    I really appreciate your thoughts on this.

    I understand the need for default sanitization. And believe me, this is what I also want to add to the plugin.

    You also pointed out a very important point (point 1), which makes me not pushing the santization so hard. Meta Box is now actively used on 400k+ websites. Any change we make will affect these huge amount of websites. I can't notify the developers (there's no way to let them know because WordPress.org doesn't provide any way to contact to them). So, I decide to not implement a forced / opinionated sanitization at the moment. I have to be very careful about this.

    Another thing that people should consider is: Meta Box is not an end-user tools. It's a tool for developers which provides API. So there's nothing like Meta Box --> Users, but it's more like Meta Box --> Developers --> Users. Because of this, developers understand which kind of data they want their users to enter. And thus, a sanitization callback fits this situation.

    If you're building a new plugin / solution using Meta Box, why don't you implement this with just a few lines of code instead of depending the default sanitization which might not work smoothly in all cases?

    There are also a few things regarding sanitization in WordPress, that I think worth mentioning:

    • Admins can enter script and style in the post content, regardless the santiziation of wp_kses_post.
    • Customizer API requires (not strictly) developers to enter santiziation callback. If no sanitization callback, then what users enter will be used.
    • wp_insert_post doesn't do any sanitization for the content when inserting the post programmatically.

    Anyway, there is a default sanitization for some fields (email, url, file_input, checkboxes). For other fields, due to the nature of the complexity, I haven't implemented that. If you have any idea on which proper sanitization for each field type, please let me know.

    #15668
    Rao AbidRao Abid
    Participant

    Hi Anh,

    Thanks for the reply.

    I am a plugin developer myself and Problem 1 was the main concern when you push something like this that can affect current users.

    i myself has written this helper class:
    https://github.com/boospot/boo-settings-helper

    and also author of this course:
    https://www.udemy.com/wordpress-plugin-development-using-boilerplate/

    I can well understand your situation as the authour of this framework and also the grievance of the situation where current 400k+ sites are using this framework without sanitization.

    I bet more than 50% of these developers are not using the sanitization filter you have referred before, simply due to one reason: They are using this framework for its "plug-and-play" nature so that they dont have to worry about setting up (explicit), saving (explicit) and sanitization (implicit) of metaboxes.

    Since its an established framework, so it will (soon) become the next target of Security researchers and i dont want metabox.io to make to headlines as some plugins have already done like "Revolution Slider", "Social Media Buttons", "Yellow Pencils" and many more as pointed here: https://www.pluginvulnerabilities.com/blog/

    Any such news can seriously undermine the reputation of the framework.

    In my opinion, following solutions are on the table:

    1. We provide a value like this for default sanitization:
      'sanitize_callback => 'default'
      Any developer using this will get default sanitization as per the nature of the field.
      [Workable but has extra typing for each field defination and make the repeating code]
    2. We can provide a constant based sanitiation like a constant is defined in wp-config.php or the plugin piggy back to the framework. if that constant is there, default sanitization will prevail.
      Example: METABOX_FIELDS_SANITIZE_DEFAULT = true
      [Most Desirable as default sanitization behavior can kick-in with just one Constant definition]

    3. We push this change is major future realeas and let the users know that its coming and they can modify their code accordingly. WooCommerce does this all the time and Developers are required to make their code compatible.
      [This solutin is most secure and will definitly raise support tickets once pushed but its the best path to follow. Any person not using default sanitization should conciously explicitly configurs in the field config array]

    Let me know your thoughts on this.

    Cheers,

    Rao

    #15698
    Anh TranAnh Tran
    Keymaster

    Hey guys,

    I added some sanitization callbacks. More will come later. The full code of the sanitization is here.

    This is the 1st step on this. Any feedback is welcomed.

    #15705
    david.hdavid.h
    Participant

    Hi Anh,

    Having dry-run the code, I pleased to see this heading in the right direction. Remember to keep this on your to-do list, because security should not be a one-time "set it and forget it" thing, but a constant test, evolution, improvement and refinement of the requirements.

    Before release, I suggest you employ some white-hat tactics and ask a core-team (maybe from your Facebook group) to try thwarting or circumventing the sanitization using combinations of complex field groups, custom tables, front-end posting and carefully malformed data.

    Thanks,
    David

    #15706
    Anh TranAnh Tran
    Keymaster

    Hi David,

    Glad you like it. I've just pushed the finished code to the "sanitize" branch. The full code is now available here.

    There are also some things left that I would love to hear your feedback:

    • textarea field: at the moment, I force to use wp_kses_post as default. But as I said in a reply earlier, some developers make this field available for users to enter scripts (such as Google Analytics or Google Adsense). What do you think best in this way? Is forcing a callback is good?
    • Custom field type are not sanitized by default. Developers are required to sanitize themselves. Do you think it's fine? Or forcing a default sanitize callback?
Viewing 15 posts - 1 through 15 (of 30 total)
  • You must be logged in to reply to this topic.