- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3
Support dynamically instantiated UI5 controls placed at a DOM tree #240
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
          
     Open
      
      
            jeongsoolee09
  wants to merge
  22
  commits into
  main
  
    
      
        
          
  
    
      Choose a base branch
      
     
    
      
        
      
      
        
          
          
        
        
          
            
              
              
              
  
           
        
        
          
            
              
              
           
        
       
     
  
        
          
            
          
            
          
        
       
    
      
from
jeongsoolee09/fix-ui5-fn
  
      
      
   
  
    
  
  
  
 
  
      
    base: main
Could not load branches
            
              
  
    Branch not found: {{ refName }}
  
            
                
      Loading
              
            Could not load tags
            
            
              Nothing to show
            
              
  
            
                
      Loading
              
            Are you sure you want to change the base?
            Some commits from the old base branch may be removed from the timeline,
            and old review comments may become outdated.
          
          Conversation
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
    A TODO is to model `placeAt` and the DOM hierarchy between instantiated controls.
The latter is a bad idea as it misses some local data flow steps.
…ty/codeql-sap-js into jeongsoolee09/fix-ui5-fn
…HTMLControlPlacedAtDom`
…ty/codeql-sap-js into jeongsoolee09/fix-ui5-fn
  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment
  
      
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
What This PR Contributes
The points are listed in the order of importance.
1. Cover XSS happening in dynamically instantiated Input and HTML
Consider this UI5 application fitting in a single index.html file:
When a button is pressed, a user-provided value is read from the
Inputcontrol and is fed to anHTMLcontrol. Usually the three controls,Input,Button, andHTMLwould all be defined in a view file (e.g. App.view.xml) and the user-provided data would be wired together using a model (e.g. JSON model) attached to the view via a binding path, and controller holding them altogether (e.g. App.controller.js). Our query was geared towards this "full application" with the standard MVC parts, and was failing to capture the XSS happening as in above. This PR aims to cover cases such as above where XSS happens without the use of model + binding paths.Bifurcate UI5Control into UI5InputControl (controls capable of receiving user input) and UI5HTMLControl (
sap.m.HTML)The previous MaD models did not differentiate UI5 library controls that are capable of receiving user input (various input controls) and one that displays them (
sap.m.HTML). This created problems trying to model an instantiation ofsap.m.HTML(HTMLControlInstantiation), and those of various input controls (InputControlInstantiation). Therefore, we differentiate them.Add an auxiliary data flow configuration
TrackPlaceAtCallConfigThis is a helper configuration later used in
DataFromInstantiatedAndPlacedAtControl(a remote flow source) andDynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDom(a new type of HTML sink we add in this PR).If there are no calls to
placeAtin the above code, XSS cannot happen because those controls won't be shown in the DOM and be presented to the user in the browser screen in the first place. Therefore, in addition to capturing instantiations to the input controls and HTML control, we check if the instantiated controls are place in a DOM by looking for aplaceAtmethod called on them.Add a unit test for
DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDomIn the sample controller
javascript/frameworks/ui5/test/models/dynamic_write_to_html_content/webapp/controller/app.controller.js, we record what we aim to find and to not find. All the unsafe code locations have one thing in common: the untrusted data is set without any use of model + binding paths.Add
DynamicallySetElementValueOfInstantiatedHTMLControlPlacedAtDomThis class aims to find writes to the
contentproperty of an instantiated HTML control. It corresponds to the locations found in thedoSomething1method in the sample controller.Add
DynamicallySetElementValueOfHTMLControlReferenceThis class aims to find write to the
contentproperty of a reference to an HTML control that is statically declared in the XML view. Similar to the above, it does not invovle any model + binding paths.2. Add UI5 customizations for the default query
XssThroughDomThere may be UI5 applications that write to unsafe property to certain default JavaScript sinks, such as
Element.innerHTML(for some testing reasons). These are sinks of the default queryXssThroughDomthat belongs to thejavascript/security-extendedsuite. However, for that query to run smoothly on UI5 applications it needs some UI5-specific customizations, mainly (1) remote flow sources and (2) summary steps.This PR defines them in the
ui5.model.ymlextension file, in the section that contributes to thesourceModelandsummaryModelofcodeql/javascript-all. The definitions are provisioned to the default queries by virtue of them contributed directly tocodeql/javascript-all.3. Improvements to QL code
There are two major anti-patterns that are replaced. Rewriting these makes the models cover equivalent cases that are equivalent in terms of data flow, yet different in terms of AST.
1. flowsTo / getALocalSource + getMethodName
These show up in the (mutually equivalent) form of:
or
All of the above can be rewritten as
2. A.getArgument(0).getALocalSource().asExpr().(StringLiteral).getValue()
This pattern demands that the expression the argument originates from is a string literal. However, the string argument can be dynamically computed (e.g. concatenation). These string operations are covered via
DataFlow::Node.getStringValue/0, so use that:It is much easier to read and understand than the original code.
4. Code cleanup
Remove unused type
TSapElementandSapElement; they only caused confusion and served no particular function.5. Stylistic improvments
Reflow comments and change inline ones to module-level QLDocs.
Future Works
Document more parts of the code as the PR review proceeds.