Skip to content

VIP feedback #1260

New issue
New issue
Open
Open
VIP feedback#1260
@jtsternberg

Description

@jtsternberg
jtsternberg
opened on Apr 14, 2019

@GaryJones has provided some feedback to get CMB2 in a better state for use on VIP sites. I'm going to list his feedback here.

Hey - here’s the first real batch of CMB2 issues I’ve found (basically from one file). See what you think.

  • includes/CMB2:365 - should be sanitize_html_class, and then wrap in esc_attr().
  • includes/CMB2:511 - needs esc_html() around $label
  • includes/CMB2:514 - needs esc_html() around $description
  • includes/CMB2:529 - needs esc_html() or wp_kses_*() around button content
  • includes/CMB2:574 - potentially incorrect use of sanitize_html_class()
  • includes/CMB2:574 - no need for strip_tags, as sanitize_text_field() does that already.
  • includes/CMB2:596 - id attribute value needs esc_attr()
  • includes/CMB2:596 - data-iterator attribute value needs esc_attr()
  • includes/CMB2:599 - data-selector attribute value needs esc_attr()
  • includes/CMB2:604 - h3 content needs esc_html()
  • includes/CMB2:627 - data-selector attribute value needs esc_attr()
  • includes/CMB2:627 - data-selector attribute value needs esc_attr()
  • includes/CMB2:627 - needs esc_html() or wp_kses_*() around button content
  • includes/CMB2:1011 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1016 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1021 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1033 - $_REQUEST input needs sanitization - absint(), sanitize_*(), etc.
  • includes/CMB2:1199 - $_GET['page'] input needs sanitization - sanitize_text_field(), etc. though somewhat mitigated by options_page_keys()
  • includes/CMB2:1203 - $_POST['action'] input needs sanitization - sanitize_text_field(), etc. though somewhat mitigated by options_page_keys()
  • includes/CMB2:1206 - The above two items could be mitigated if only a boolean is returned.
  • includes/CMB2:1718 - Nonce isn’t calculated with current time, so could be bypassed with replay attacks > 10 minutes after it is generated. Use wp_create_nonce() instead.

Here’s more:

  • includes/CMB2_Ajax.php:79 - use (int) instead of intval() - casting is about six times quicker.
  • includes/CMB2_Ajax.php:82 - Escaping is typically only done on output - did you mean rawurlencode() or something else?
  • includes/CMB2_Ajax.php:94 - Object ID is not sanitized.
  • includes/CMB2_Ajax.php:95 - Object type is not sanitized.
  • includes/CMB2_Ajax.php:95 - Field ID is not sanitized.
  • includes/CMB2_Ajax.php:181 - oembed needs escaping
  • includes/CMB2_Ajax.php:181 - field ID arg needs escaping
  • includes/CMB2_Base.php:456 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:456 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:456 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Base.php:458 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:458 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:458 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Base.php:462 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:462 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:462 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Base.php:464 - Either wrap i18n string in wp_kses_post(), or remove <strong> and use esc_html__().
  • includes/CMB2_Base.php:464 - Placeholder replacement variables should be escaped.
  • includes/CMB2_Base.php:464 - Consider using an Exception or WP_Error instead of trigger_error().
  • includes/CMB2_Field.php:906 - Attribute values need escaping
  • includes/CMB2_Field.php:916 - Label need escaping with wp_kses_post()
  • includes/CMB2_Field.php:966 - Label attributes need escaping
  • includes/CMB2_Field.php:1119 - Output need escaping
  • includes/CMB2_Field.php:1125 - Attribute values need escaping
  • includes/CMB2_Field.php:1415 - Escaping here is premature. If these are defaults, presumably they can be overridden, and any overridden values should be escaped later anyway.
  • includes/CMB2_Field.php:1416 - Escaping here is premature. If these are defaults, presumably they can be overridden, and any overridden values should be escaped later anyway.
  • includes/CMB2_Field.php:1448 - Escaping here feels premature. Why is the static string escaped, but the dynamic value not? Leave escaping until the context is known.
  • includes/CMB2_Field.php:1452 - Escaping here feels premature. Why is the static string escaped, but the dynamic value not? Leave escaping until the context is known.
  • includes/CMB2_Field_Display.php:132 - sanitize_html_class() and esc_attr() needed here.
  • includes/CMB2_Field_Display.php:135 - List item content needs escaping.
  • includes/CMB2_Field_Display.php:151 - print_r() is considered code for debugging - is there not something better that can be used?
  • includes/CMB2_Field_Display.php:162 - The whole output should be escaped after any URLs have been converted to anchor links - wp_kses_post().
  • includes/CMB2_Field_Display.php:174 - Echo’d content needs escaping.
  • includes/CMB2_Field_Display.php:214 - Fallback needs escaping.
  • includes/CMB2_Field_Display.php:216 - Options value needs escaping.
  • includes/CMB2_Field_Display.php:218 - How do you know that esc_attr() is the correct context to escape with here?
  • includes/CMB2_Field_Display.php:241 - How do you know that esc_attr() is the correct context to escape with here?
  • includes/CMB2_Field_Display.php:245 - Output needs escaping.
  • includes/CMB2_Field_Display.php:245 - Switch wpautop() and wp_kses_post() calls.
  • includes/CMB2_Field_Display.php:267 - <xmp> has been deprecated since HTML 3.2, and removed completely from HTML 5.
  • includes/CMB2_Field_Display.php:278 - Timestamp output needs escaping.
  • includes/CMB2_Field_Display.php:289 - Timestamp output needs escaping.
  • includes/CMB2_Field_Display.php:318 - Timestamp output needs escaping.
  • includes/CMB2_Field_Display.php:379 - Taxonomy should be run through sanitize_html_class().
  • includes/CMB2_Field_Display.php:424 - URL needs escaping with esc_url().
  • includes/CMB2_Field_Display.php:432 - Change esc_html__ to __().
  • includes/CMB2_Field_Display.php:433 - URL needs escaping with esc_url().
  • includes/CMB2_Field_Display.php:434 - File name needs escaping.
  • includes/CMB2_Field_Display.php:457 - Escaping needed.
  • includes/CMB2_Options_Hookup.php:199 - Option key should be run through sanitize_html_class() and esc_attr().
  • includes/CMB2_Options_Hookup.php:199 - ID value should escaped with esc_attr() (or absint() if integer).
  • includes/CMB2_Options_Hookup.php:280 - Nonce check needed, to ensure that the submission came from an authorised source.
  • includes/CMB2_Options_Hookup.php:294 - Possible to use any sort of whitelist approach here, to avoid trying to process malicious data in $_POST?
  • includes/CMB2_Sanitize.php:384 - Why serialize? Serialized data has known vulnerability problems with Object Injection. JSON is generally a better approach for serializing data. See https://www.owasp.org/index.php/PHP_Object_Injection
  • includes/CMB2_Sanitize.php:416 - Would sanitize_textarea_field be better here?
  • includes/CMB2_Types.php:338 - Table ID needs escaping with esc_attr().
  • includes/CMB2_Types.php:344 - Table ID needs escaping with esc_attr().
  • includes/CMB2_Types.php:401 - Classes should be escaped.
  • includes/CMB2_Types.php:435 - Description in span should be escaped.
  • includes/CMB2_Types.php:463 - Unlike _name(), which can be escaped when it is called, the _id() methods adds multiple attribute values, so each should be escaped here.
  • includes/CMB2_hookup.php:461 - ID attribute value should be escaped.
  • includes/CMB2_hookup.php:466 - Span output should be escaped.
  • includes/CMB2_hookup.php:470 - Escaping should be with esc_html(), not esc_attr().
  • includes/CMB2_hookup.php:618 - User ID needs to be sanitized.
  • includes/types/CMB2_Type_Checkbox.php:57 - Double-check that all placeholder replacements are escaped.
  • includes/types/CMB2_Type_File.php:82 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_File.php:106 - src attribute needs to be escaped with esc_url().
  • includes/types/CMB2_Type_File_Base.php:63 - rel attribute needs to be escaped with esc_attr().
  • includes/types/CMB2_Type_File_Base.php:80 - File name string needs to be escaped with esc_html().
  • includes/types/CMB2_Type_File_Base.php:81 - Link value needs to be escaped with esc_url().
  • includes/types/CMB2_Type_File_Base.php:83 - rel attribute needs to be escaped with esc_attr().
  • includes/types/CMB2_Type_File_List.php:41 - Button value should not be escaped with esc_attr() if rendering as <button>, or esc_html__() should be __() if rendering as <input type="button">.
  • includes/types/CMB2_Type_File_List.php:46 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_Multi_Base.php:23 - value attribute should be escaped.
  • includes/types/CMB2_Type_Multi_Base.php:44 - for attribute should be escaped.
  • includes/types/CMB2_Type_Multi_Base.php:44 - Label content should be escaped.
  • includes/types/CMB2_Type_Oembed.php:46 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_Oembed.php:46 - div content should be escaped / wp_kses_post().
  • includes/types/CMB2_Type_Title.php:41 - Span content (name) needs escaping.
  • includes/types/CMB2_Type_Wysiwyg.php:87 - ID attribute value should be escaped.
  • includes/types/CMB2_Type_Wysiwyg.php:89 - Several attribute values should be escaped.
  • js/cmb2.js:58 - HTML string concatenation detected, this is a security risk, use DOM node construction or a templating language instead.
  • js/cmb2.js:92 - HTML string concatenation detected, this is a security risk, use DOM node construction or a templating language instead.
  • js/cmb2.js:320 - Any HTML passed to append gets executed. Make sure it’s properly escaped.
  • js/cmb2.js:345 - Any HTML passed to html gets executed. Make sure it’s properly escaped.
  • js/cmb2.js:560 - Any HTML passed to html gets executed. Make sure it’s properly escaped.
  • js/cmb2.js:973 - HTML string concatenation detected, this is a security risk, use DOM node construction or a templating language instead.
  • js/jquery-ui-timepicker-addon.min.js - include an unminified script for review and debugging.

The final bit are not security issues, but just some observations I made as I was reviewing the code:

  • includes/CMB2_Ajax.php:181 - For accessibility, consider using a button, not an empty anchor.
  • includes/CMB2_Ajax.php:207 - What is this magic number?
  • includes/CMB2_Ajax.php:233 - What is this magic number?
  • includes/CMB2_Field_Display.php:155 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:166 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:178 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:189 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:200 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:223 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:249 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:260 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:271 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:282 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:293 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:322 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:348 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:386 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:441 - Each class should be in its own file.
  • includes/CMB2_Field_Display.php:463 - Each class should be in its own file.
  • includes/CMB2_Options.php:44 - Each class should be in its own file.
  • includes/CMB2_Utils.php:298 - Why not return ! CMB_Utils::isempty( $value ); ?
  • includes/CMB2_REST_Controller.php:4 - Is WP_REST_Controller shim still needed?
  • includes/types/CMB2_Type_Textares_Code.php:25 - Why not move the </pre> to line 36 to be more logical?

There were quite a few instances like:

echo '<button type="button" data-selector="', $field_group->id(), '_repeat" data-confirm="', esc_attr( $confirm_deletion ), '" class="dashicons-before dashicons-no-alt cmb-remove-group-row" title="', esc_attr( $field_group->options( 'remove_button' ) ), '"></button>';

That could be made easier to read (and ensure escaping), like:

    '<button type="button" data-selector="%s" data-confirm="%s" class="dashicons-before dashicons-no-alt cmb-remove-group-row" title="%s"></button>',
    esc_attr( $field_group->id() . _repeat ),
    esc_attr( $confirm_deletion ),
    esc_attr( $field_group->options( 'remove_button' ) )
);```

(which some of the later code I was reviewing did do)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions